Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reduce code duplication in find* functions #36965

Merged
merged 1 commit into from
Nov 12, 2021
Merged

Conversation

goretkin
Copy link
Contributor

@goretkin goretkin commented Aug 7, 2020

Try to address #36963

Also, whatever fix arises from #36938 will only need to be applied in a fewer number of places.

I expect the compiler can certainly specialize on identity. Is it necessary to force inlining?

@goretkin
Copy link
Contributor Author

goretkin commented Aug 7, 2020

predicate and non-predicate versions were introduced by #25577

@nalimilan
Copy link
Member

Sounds good, but can you run some benchmarks and look at the generated code to check that this doesn't introduce regressions?

@goretkin
Copy link
Contributor Author

goretkin commented Aug 8, 2020

Yeah, it would be good to have a sense that there aren't performance regressions. Can nanosoldier help here?

@nalimilan
Copy link
Member

Yes, if there are tests in BaseBenchmarks for this (haven't checked) or if you make a PR to add them. But a quick local check would be enough IMHO. You can also check that the generated code is the same (which is likely).

@goretkin
Copy link
Contributor Author

goretkin commented Aug 8, 2020

Looks like there are some: https://github.com/JuliaCI/BaseBenchmarks.jl/blob/ce11d8badcb0e9f6d8b0518ffd57a472128af930/src/find/FindBenchmarks.jl#L39

but I'll take a look at the generated code, since you're probably right that it didn't change.

@vtjnash vtjnash merged commit 8cf89d7 into JuliaLang:master Nov 12, 2021
@goretkin
Copy link
Contributor Author

FWIW, I wanted a way to programmatically compare the generated code, and I didn't know how to do that since all the tools seem to print to stdout directly. Perfect was the enemy of good, but I'm glad to see this merged!

@bkamins bkamins mentioned this pull request Nov 14, 2021
vtjnash pushed a commit that referenced this pull request Nov 17, 2021
Fixes #43078; This problem was introduced by #36965
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Feb 22, 2022
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Feb 22, 2022
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants