-
-
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
inference×effects: improve the const-prop' heuristics #46471
base: avi/inlining_cost
Are you sure you want to change the base?
Conversation
@nanosoldier |
I'm not particularly opposed to it, but it does stretch the definition of effects a fair bit. This is more like reverse dataflow information, similar to escape analysis. That said, I don't think there's any big problem with this either. |
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. |
Very cool! If I understand correctly it should also solve #44330 |
How will this work for functions with more than 32 inputs? |
Good catch! I'd add a new test case based on the issue.
No, currently this only works for <=8 arguments. We may want to extend it though. |
48b4957
to
6059860
Compare
@nanosoldier |
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. |
6059860
to
db2a1e8
Compare
f8ebc9f
to
ab6a268
Compare
@nanosoldier |
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. |
ab6a268
to
500c6b9
Compare
500c6b9
to
6661563
Compare
6661563
to
df74396
Compare
df74396
to
36b9be3
Compare
@nanosoldier |
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. |
36b9be3
to
5259e28
Compare
@nanosoldier |
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. |
5259e28
to
2939699
Compare
2939699
to
c34e6e0
Compare
9c09477
to
0a59892
Compare
@nanosoldier |
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. |
This commit improves the heuristics to judge const-prop' profitability with new effect property `:const_prop_profitable_args`. This is supposed to supplement our primary const-prop' heuristic based on inlining cost and is supposed to be a general fix for type stabilities issues discussed at e.g. #45952 and #46430 (and eliminating the need for manual `@constprop :aggressive` clutters in such situations). The new effect property `:const_prop_profitable_args` tracks call arguments that can be considered to shape up generated code if their constant information is available. Currently this commit exploits the following const-prop' profitabilities: - `Val(x)`-profitability: as `Val` generally encodes constant information into the type domain, it is generally profitable to constant prop' `x` if the constructed `Val(x)` is used later (e.g. for dispatch). This basically tries to exploit const-prop' profitability in the following kind of case: ```julia kernel(::Val{1}, args...) = ... kernel(::Val{2}, args...) = ... function profitable1(x::Int, args...) kernel(Val(x), args...) end ``` This allows the compiler to perform const-prop' for case like #45952 even if the primary heuristics based on inlining cost gets confused. - branching-profitability: constant branch condition is generally very profitable as it can shape up generated code as well as narrow down the return type inference by cutting off the dead branch. ```julia function profitable2(raise::Bool, args...) v = op(args...) if v === nothing && raise return nothing end return v end ``` Currently this commit passes all the test cases and also actually improves target type stabilities, but doesn't work very ideally as it seems to be a bit too aggressive (this commit right now strictly increases the chances of const-propagation). I'd like to further tweak this heuristic to keep the latency in general cases.
7803de2
to
ad06a5c
Compare
0a59892
to
87c2cf2
Compare
This commit improves the heuristics to judge const-prop' profitability
with new effect property
:const_prop_profitable_args
. This is supposedto supplement our primary const-prop' heuristic based on inlining cost
and is supposed to be a general fix for type stabilities issues discussed
at e.g. #45952 and #46430 (and eliminating the need for manual
@constprop :aggressive
clutters in such situations).The new effect property
:const_prop_profitable_args
tracks callarguments that can be considered to shape up generated code if
their constant information is available. Currently this commit
exploits the following const-prop' profitabilities:
Val(x)
-profitability: asVal
generally encodes constant informationinto the type domain, it is generally profitable to constant prop'
x
if the constructed
Val(x)
is used later (e.g. for dispatch).This basically tries to exploit const-prop' profitability in the
following kind of case:
[x;;]
#45952even if the primary heuristics based on inlining cost gets confused.
profitable as it can shape up generated code as well as narrow down
the return type inference by cutting off the dead branch.
Currently this commit passes all the test cases and also actually
improves target type stabilities, but doesn't work very ideally as it
seems to be a bit too aggressive (this commit right now strictly
increases the chances of const-propagation). I'd like to further tweak
this heuristic to keep the latency in general cases.
fixes #45952, closes #46430.
/cc @Keno Do you have any opinion on this?