JIT: Switch StaysWithinManagedObject to peel offsets from VNs #105169
Conversation
The SCEV analysis does not care about the value of something once it is seen to be invariant inside the loop we are currently analyzing. This was problematic for this logic that tries to peel additions away from offsets; for arm64, we may have hoisted `array + 0x10` outside the loop, which would cause us to fail to get back to the base array. Switch the reasoning to use VNs and peel the offsets from the VNs instead. No x64 diffs are expected as we do not hoist the `array + 0x10` out of the loop there. Improvements expected on arm64 where we can now prove that a "full" strength reduction is allowable more often.
ghost added the area-CodeGen-coreclr 
| Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
| cc @dotnet/jit-contrib PTAL @AndyAyersMS Seeing good arm64 diffs from this locally: https://gist.github.com/jakobbotsch/dc38a954c2c6027f469ad1b8324831bf Looks like JIT-EE GUID changed and the collection isn't done so probably won't get any CI diffs here until tomorrow. |
| | ||
| *offset = 0; | ||
| VNFuncApp app; | ||
| while (GetVNFunc(*vn, &app) && (app.m_func == VNF_ADD)) |
There was a problem hiding this comment.
Seems like VN ought to commute so that constants are (say) rightmost / and or fold chains of constant adds... but I take it you're seeing add chains or mixtures of constants left and right.
There was a problem hiding this comment.
To be honest I haven't checked whether we see cases that require the extra unwrapping... I just wrote this one fully generally to match gtPeelOffsets and Scev::PeelAdditions.
We do canonicalize commutative operators, but only in terms of raw VN number, keeping the smallest VN number as op1. Definitely seems like we could make sure constants go to the right.
There was a problem hiding this comment.
Definitely seems like we could make sure constants go to the right.
I remember I tried that but found no diffs
| Final diffs here: https://dev.azure.com/dnceng-public/public/_build/results?buildId=748554&view=ms.vss-build-web.run-extensions-tab |
3 participants
The SCEV analysis does not care about the value of something once it is seen to be invariant inside the loop we are currently analyzing. This was problematic for this logic that tries to peel offsets away from an array; for arm64, we may have hoisted
array + 0x10outside the loop, which would cause us to fail to get back to the base array.Switch the reasoning to use VNs and peel the offsets from the VNs instead.
No x64 diffs are expected as we do not hoist the
array + 0x10out of the loop there. Improvements expected on arm64 where we can now prove that a "full" strength reduction is allowable more often.