JIT: Improve and fix StaysWithinManagedObject
#105108
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- For string accesses we also produce `ARR_ADDR`, so we must take care to use `GenTreeArrAddr::GetFirstElemOffset` instead of hardcoding `OFFSETOF__CORINFO_Array__data` - There are cases where VN is fully able to prove that bound < ARR_LENGTH(vn), specifically when the array is stored in a static readonly field. In those cases everything reduces to constants, so allow VN to try to prove it but fall back to our manual logic otherwise. - Rephrase the fallback as a VN test as well. In a standard `for (;i < arr.Length;)` loop we have a bound on the backedge of the shape `ARR_LENGTH(array) - 1`. The previous strategy was to syntactically check if the LHS was such an array length on the same array as the base of the add recurrence. Instead of doing that, we can ask more generally for any shape `x - c` whether we know that `x <= ARR_LENGTH(array)`. In the usual case of `x == ARR_LENGTH(array)` this is trivially true and VN knows that. However, there are other cases where this is provable by RBO due to a dominating compare; particularly loop cloning introduces these dominating compares when cloning loops of the shape `for (; i < n;)`. This fixes dotnet#105087.
ghost
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
This was referenced Jul 18, 2024
|
cc @dotnet/jit-contrib PTAL @AndyAyersMS Diffs with strength reduction enabled. Surprisingly we have significantly fewer hits here for arm64 -- I wonder why that is. Will take a closer look tomorrow, but in any case this PR should be ok as is. |
AndyAyersMS
approved these changes
Jul 19, 2024
The reason is that on arm64 we frequently hoist out the |
21 tasks
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
For string accesses we also produce
ARR_ADDR, so we must take care to useGenTreeArrAddr::GetFirstElemOffsetinstead of hardcodingOFFSETOF__CORINFO_Array__dataThere are cases where VN is fully able to prove that bound < ARR_LENGTH(vn), specifically when the array is stored in a static readonly field. In those cases everything reduces to constants, so allow VN to try to prove it but fall back to our manual logic otherwise.
Rephrase the fallback as a VN test as well. In a standard
for (;i < arr.Length;)loop we have a bound on the backedge of the shapeARR_LENGTH(array) - 1. The previous strategy was to syntactically check if the LHS was such an array length on the same array as the base of the add recurrence.Instead of doing that, we can ask more generally for any shape
x - cwhether we know thatx <= ARR_LENGTH(array). In the usual case ofx == ARR_LENGTH(array)this is trivially true and VN knows that. However, there are other cases where this is provable by RBO due to a dominating compare; particularly loop cloning introduces these dominating compares when cloning loops of the shapefor (; i < n;). This fixes JIT:StaysWithinManagedObjectshould be able to handle more cloned loops patterns #105087.