duplicate invoke expr during call method splitting #22481
Conversation
|
Will this help the inlining issue I reported in #22097? |
|
This does the transform mentioned at #22097 (comment) so it no longer needs to be done manually. |
vtjnash
force-pushed
the
jn/split-invoke
branch
2 times, most recently
from
f299e73 to
4315b7a
Compare
|
@nanosoldier |
|
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels |
|
forget to update the inliner heuristics for the new code structure. lets try that again: @nanosoldier |
|
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels |
|
Alright, the performance changes here are now unhelpfully coupled due to its affect on inlining. But I don't see anything that seems broken. |
|
what does "unhelpfully coupled" mean? |
|
That performance changes are because of the different inlining heuristics and not what the PR is actually fixing? |
|
How about adding a test for the fix then? |
|
Oh, and the regressions in the |
Almost all of the difference are attributable to getting the inlining decision wrong (#22210), but neither the old nor the new is consistently better. But some of the differences (at least one improvement – raytrace) are real.
The PR doesn't fix anything. It's just a more concise and readable way to implement this, and moves us closer to a very-linear-ir (so that flipping that switch will have a smaller impact). |
With the new gc-rooting pass, this should generate better code over the copying of just the method instance.
these aren't usually performance-sensitive functions, and llvm is more likely to miscompile them when these functions get large
|
Removed the inlining changes, so hopefully this PR will not be unhelpfully coupled anymore: @nanosoldier |
|
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels |
|
Looks excellent. Will merge tomorrow if there's no comments. |
|
many of the nullable operations got considerably slower, is this inhibiting simd? |
|
Those functions don't run this codepath, and are generating the same IR when I examined them locally. Not sure why they are showing up noisy here. |
|
They're not the usual suspects for being (that) noisy, so @nanosoldier |
|
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels |
|
Good enough for me, sqrtm is a frequently noisy one |
|
The nullable operations had gotten faster on master. |
|
nanosoldier should be building the merge commit though so that shouldn't make a difference |
With the new gc-rooting pass, this should generate better code over the copying of just the method instance.