-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Make Compiler use separate sorting algorithm. #47066
Conversation
This is a combination of
The algorithmic changes in #46586 and #46587 have little practical performance impact, but what impact there is is positive. I think what is holding this back right now is that we need review of those changes by someone familiar with the appropriate sections of the compiler. |
@nanosoldier |
Your benchmark job has completed - no performance regressions were detected. A full report can be found here. |
Co-authored-by: Valentin Churavy <vchuravy@users.noreply.github.com>
Co-authored-by: Hendrik Ranocha <ranocha@users.noreply.github.com>
3533c60
to
1ab8885
Compare
@testset "many basic blocks" begin | ||
n = 1000 | ||
ex = :(return 1) | ||
for _ in 1:n | ||
ex = :(if rand()<.1 | ||
$(ex) end) | ||
end | ||
@eval begin | ||
function f_1000() | ||
$ex | ||
return 0 | ||
end | ||
end | ||
@test f_1000()===0 | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused at this test case. What's being tested here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that this is testing compilation of a function with a bunch of basic blocks. Is there a better version of this test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the compilation performance being tested, or is there any correctness issue? And why f_1000
always returns 0
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a correctness issue (#47065) where the compiler would try to sort a big list and fail. f_1000
doesn't technically always return 0, but it returns 1 1 in 10^1000 times which is close enough to never that it will never fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I understand. I'd prefer not having @testset
there though.
|
||
# Make sure that the compiler can sort things. | ||
# https://github.com/JuliaLang/julia/issues/47065 | ||
@testset "Compiler Sorting" begin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is redundant with (or at least should be in the same place as) test/compiler/sort.jl
This commit refines `interpreter_exec.jl` further, on top of #53218. In detail, by using `Base.Experimental.@compiler_options compile=min`, it's now possible to mimic the effect of `julia --compile=[min|yes]`. This commit is leveraging it and includes `interpreter_exec.jl` from `test/compiler/ssair.jl`, allowing us to get better stack traces and test summaries. In addition, I've introduced several tests concerning the `src.inferred` values and have relocated the tests from #47066 within `interpreter_exec.jl` to `test/compiler/irpasses.jl`, since it is related to Julia-level compilation and not to interpreter execution.
This commit refines `interpreter_exec.jl` further, on top of #53218. In detail, by using `Base.Experimental.@compiler_options compile=min`, it's now possible to mimic the effect of `julia --compile=[min|yes]`. This commit is leveraging it and includes `interpreter_exec.jl` from `test/compiler/ssair.jl`, allowing us to get better stack traces and test summaries. In addition, I've introduced several tests concerning the `src.inferred` values and have relocated the tests from #47066 within `interpreter_exec.jl` to `test/compiler/irpasses.jl`, since it is related to Julia-level compilation and not to interpreter execution.
This commit refines `interpreter_exec.jl` further, on top of #53218. In detail, by using `Base.Experimental.@compiler_options compile=min`, it's now possible to mimic the effect of `julia --compile=[min|yes]`. This commit is leveraging it and includes `interpreter_exec.jl` from `test/compiler/ssair.jl`, allowing us to get better stack traces and test summaries. In addition, I've introduced several tests concerning the `src.inferred` values and have relocated the tests from #47066 within `interpreter_exec.jl` to `test/compiler/irpasses.jl`, since it is related to Julia-level compilation and not to interpreter execution.
This commit refines `interpreter_exec.jl` further, on top of #53218. In detail, by using `Base.Experimental.@compiler_options compile=min`, it's now possible to mimic the effect of `julia --compile=[min|yes]`. This commit is leveraging it and includes `interpreter_exec.jl` from `test/compiler/ssair.jl`, allowing us to get better stack traces and test summaries. In addition, I've introduced several tests concerning the `src.inferred` values and have relocated the tests from #47066 within `interpreter_exec.jl` to `test/compiler/irpasses.jl`, since it is related to Julia-level compilation and not to interpreter execution.
Separated from #46679 as the easiest way to fix #47065. Needs tests.
closes #46586
closes #46587