Enable inference on throw blocks #47595
Conversation
|
I feel like this is lacking some context. Clearly, the current choice was put there for some reason (#35982). So what has changed? What is the argument for spending inference time on code that will throw? Also, I don't think this change any benchmarks or package tests, it would just infer more than it currently does. If this is needed for some non-standard code usage (say GPU or similar) I think this should be applied only when explicitly enabled from a custom interpreter. |
|
What's the motivation? This feature is added and enabled intentionally since we figured out this is good for latency. We should check impacts on latency carefully. |
|
@KristofferC a lot of the motivation for this PR is that in between then and now we added the effect system and inferring tighter effects can help latency a bunch since it allows us to run constant folding more often. |
|
Did you check a TTFP or something? |
|
I haven't yet, but plan to. |
|
The core issue here is that unoptimizing error paths interacts badly with effects inference, which can cause more latency problems than it solves. We should re-evaluate the trade-off. |
brenhinkeller
added
compiler:effects
DilumAluthge
force-pushed
the
try-optimizeing-throw-blocks
branch
from
cb89830 to
3b851ff
Compare
KristofferC
changed the title
|
@oscardssmith Could you check the numbers reported in #35982 on this PR with rebasing the latest master? |
|
Just saw another case where this issue is seriously pessimizing effects. Can we do the benchmarking and see where we're at? |
My primary motivation here was to get the Xoshiro constructor to be removed if the RNG is unused (I have a struct that always has an RNG in it, which may or may not be used). I first tried to do that in a more fine-grained way, but #47595 currently makes that infeasible. Nevertheless, I figured it was worth leaving those improvements in so we can switch over once #47595 is resolved. In the meantime, annotate the top-level Xoshiro constructor as `:removable`.
oscardssmith
force-pushed
the
try-optimizeing-throw-blocks
branch
from
3b851ff to
7970bc5
Compare
|
@nanosoldier |
|
Your benchmark job has completed - successfully executed benchmarks. A full report can be found here. |
|
@nanosoldier |
|
why is |
|
Otherwise it doesn't run a comparison. It just gives you absolute values. |
|
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. |
|
I think we should get some larger benchmarks that include some runtime performance as well. Let's get the compile-time benchmarks of large packages such as Let's also get some runtime benchmarks from nanosoldier on the |
|
We should consider trying a version of it that only deoptimizes the throw blocks if and only if the effects already make it constprop ineligibile, but we should also evaluate the original motivating benchmarks from when this change was introduced. Inference mcirobenchmark time is sort of the wrong benchmark here, because we do expect it to do more work ahead of time, just potentially armotizing it later with better constprop. |
|
Replaced by #49235. |
This removes the disabling of inference on error paths. This will need careful evaluation (probably a nanosoldier and package eval)