Don't attempt concrete eval if there's no information to be gained #46774
Conversation
Keno
force-pushed
the
kf/skipconcrete
branch
from
9f7a7e2 to
c21cd78
Compare
| if is_removable_if_unused(result.effects) | ||
| if isa(result.rt, Const) || call_result_unused(sv) | ||
| # There is no more information to be gained here. Bail out early. | ||
| return nothing | ||
| end | ||
| end |
There was a problem hiding this comment.
Maybe we should implement this logic within concrete_eval_eligible? There might be a chance for const-prop' to shape up the optimized IR body even if it can't refine the type/effects information.
There was a problem hiding this comment.
The IR will not be inlined ideally. We should see it's dead and remove it
There was a problem hiding this comment.
If the IR body is inlineable but not simple enough for it to be folded into ConstAPI, it may hit this branch
julia/base/compiler/ssair/inlining.jl
Lines 1015 to 1016 in 8d9d23d
and end up being expanded into the callee.
Having said that, we intentionally miss this sort of optimization opportunity, like:
julia/base/compiler/abstractinterpretation.jl
Lines 1003 to 1008 in 86e0c6a
There was a problem hiding this comment.
I've thought about this some more, but I'm not sure it's really worth the cost to do the extra constprop or semiconcrete eval. Yes, it's possible that would shape up the IR farther, but we already have very strong effects on this. If a case comes up where this becomes an issue, I'd rather just see if there's something we can do to strengthen the optimizer.
Codecov Report
@@ Coverage Diff @@
## master #46774 +/- ##
=======================================
Coverage 93.50% 93.50%
=======================================
Files 386 386
Lines 86523 86523
=======================================
Hits 80900 80900
Misses 5623 5623 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
| @@ -887,6 +887,12 @@ function abstract_call_method_with_const_args(interp::AbstractInterpreter, | |||
| if !const_prop_enabled(interp, sv, match) | |||
| return nothing | |||
| end | |||
| if is_removable_if_unused(result.effects) | |||
There was a problem hiding this comment.
Can we also bail out if the return type is Bottom?
There was a problem hiding this comment.
I think if the rt is known to be Union{}, we need to skip concrete eval and let the regular heuristics take care of it, which disable const prop for this case unless explicitly forced (which could make sense to simplify the inlined IR).
There was a problem hiding this comment.
That's mostly unrelated to this PR though, so we should probably merge this first and then I can put up a separate PR for that change.
|
Rebased. I have some more tweaks I want to make to this logic, but let's get this in, since it should be a good improvement. |
…o large When we originally implemented concrete-eval, we decided not to create `Const` lattice elements for constants that are too large, on the fear that these would end up in the IR and blowing up the size. Now that we have some experience with this, I think that decision was incorrect for several reasons: 1. We've already performed the concrete evaluation (and allocated the big object), so we're just throwing away precision here that we could have otherwise used (Although if the call result is unused, we probably shouldn't do concrete eval at all - see #46774). 2. There's a number of other places in the compiler where we read large values into `Const`. Unless we add these kinds of check there too, we need to have appropriate guards in the optimizer and the cache anyway, to prevent the IR size blowup. 3. It turns out that throwing away this precision actually causes significant performance problems for code that is just over the line. Consider for example a lookup of a small struct inside a large, multi-level constant structure. The final result might be quite tiny, but if we refuse to propagate the intermediate levels, we might end up doing an (expensive) full-constprop when propagating the constant information could have given us a much cheaper concrete evaluation. This commit simply removes that check. If we see any regressions as a result of this, we should see if there are additional guards needed in the optimizer.
If the effects are already maximized and the rt is already Const, there is nothing concrete evaluation could possibly tell us that we don't already know - just bail early in that case to save some inference time.
…o large When we originally implemented concrete-eval, we decided not to create `Const` lattice elements for constants that are too large, on the fear that these would end up in the IR and blowing up the size. Now that we have some experience with this, I think that decision was incorrect for several reasons: 1. We've already performed the concrete evaluation (and allocated the big object), so we're just throwing away precision here that we could have otherwise used (Although if the call result is unused, we probably shouldn't do concrete eval at all - see #46774). 2. There's a number of other places in the compiler where we read large values into `Const`. Unless we add these kinds of check there too, we need to have appropriate guards in the optimizer and the cache anyway, to prevent the IR size blowup. 3. It turns out that throwing away this precision actually causes significant performance problems for code that is just over the line. Consider for example a lookup of a small struct inside a large, multi-level constant structure. The final result might be quite tiny, but if we refuse to propagate the intermediate levels, we might end up doing an (expensive) full-constprop when propagating the constant information could have given us a much cheaper concrete evaluation. This commit simply removes that check. If we see any regressions as a result of this, we should see if there are additional guards needed in the optimizer.
…o large When we originally implemented concrete-eval, we decided not to create `Const` lattice elements for constants that are too large, on the fear that these would end up in the IR and blowing up the size. Now that we have some experience with this, I think that decision was incorrect for several reasons: 1. We've already performed the concrete evaluation (and allocated the big object), so we're just throwing away precision here that we could have otherwise used (Although if the call result is unused, we probably shouldn't do concrete eval at all - see #46774). 2. There's a number of other places in the compiler where we read large values into `Const`. Unless we add these kinds of check there too, we need to have appropriate guards in the optimizer and the cache anyway, to prevent the IR size blowup. 3. It turns out that throwing away this precision actually causes significant performance problems for code that is just over the line. Consider for example a lookup of a small struct inside a large, multi-level constant structure. The final result might be quite tiny, but if we refuse to propagate the intermediate levels, we might end up doing an (expensive) full-constprop when propagating the constant information could have given us a much cheaper concrete evaluation. This commit simply removes that check. If we see any regressions as a result of this, we should see if there are additional guards needed in the optimizer.
…o large (#47283) When we originally implemented concrete-eval, we decided not to create `Const` lattice elements for constants that are too large, on the fear that these would end up in the IR and blowing up the size. Now that we have some experience with this, I think that decision was incorrect for several reasons: 1. We've already performed the concrete evaluation (and allocated the big object), so we're just throwing away precision here that we could have otherwise used (Although if the call result is unused, we probably shouldn't do concrete eval at all - see #46774). 2. There's a number of other places in the compiler where we read large values into `Const`. Unless we add these kinds of check there too, we need to have appropriate guards in the optimizer and the cache anyway, to prevent the IR size blowup. 3. It turns out that throwing away this precision actually causes significant performance problems for code that is just over the line. Consider for example a lookup of a small struct inside a large, multi-level constant structure. The final result might be quite tiny, but if we refuse to propagate the intermediate levels, we might end up doing an (expensive) full-constprop when propagating the constant information could have given us a much cheaper concrete evaluation. This commit simply removes that check. If we see any regressions as a result of this, we should see if there are additional guards needed in the optimizer.
If the effects are already maximized and the rt is already Const, there is nothing concrete evaluation could possibly tell us that we don't already know - just bail early in that case to save some inference time.