JIT: Fix loop memory dependence recording for memory defined in sibling loops#106076
Merged
Conversation
During map selection VN will keep track of memory VNs that the result
depends on. This is used so that we can know whether the value computed
by a tree inside a loop depends on any previous memory that was defined
inside the loop; if so, we cannot allow hoisting for it.
It was possible for the previous memory to be defined inside a more
nested loop. Take the following example:
```csharp
for (int i = 0; i < n; i++)
{
int j = 0;
do
{
s_otherField = [1];
} while (j < n);
s_field = true;
Use(s_field);
}
```
In this case the memory dependence of the load of `s_field` ends up
being a memory definition inside the nested loop. (One could think the
dependency would come from the `s_field = true` block, but we do not
track the memory definitions on a fine-grained basis; rather we get it
from the previous memory phi def, which is good enough for hoisting.)
The logic did not properly handle this case; if the memory definition is
in a more nested loop, we need to walk the ancestor loops to figure out
if the definition is actually happening inside one of them. If so there
is still a dependency.
Fix dotnet#105413
Member
Author
|
cc @dotnet/jit-contrib PTAL @AndyAyersMS |
AndyAyersMS
approved these changes
Aug 7, 2024
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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
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.
During map selection VN will keep track of memory VNs that the result depends on. This is used so that we can know whether the value computed by a tree inside a loop depends on any previous memory that was defined inside the loop; if so, we cannot allow hoisting for it.
It was possible for the previous memory to be defined inside a more nested loop. Take the following example:
In this case the memory dependence of the load of
s_fieldends up being a memory definition inside the nested loop. (One could think the dependency would come from thes_field = trueblock, but we do not track the memory definitions on a fine-grained basis; rather we get it from the previous memory phi def, which is good enough for hoisting.)The logic did not properly handle this case; if the memory definition is in a more nested loop, we need to walk the ancestor loops to figure out if the definition is actually happening inside one of them. If so there is still a dependency.
Fix #105413