-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
JIT: more fixes for VN loop dependence tracking #56184
JIT: more fixes for VN loop dependence tracking #56184
Conversation
Specify `Overwrite` when setting loop dependence map entries, as we may refine the initial result. Fixes dotnet#56174. Extract loop dependence of `VNF_PhiMemoryDef`. Fixes new case noted in dotnet#55936, and 13/16 or so other cases Jakob sent me privately. Also update a comment and fix tests to work better with jitstress per other notes on that PR.
cc @jakobbotsch @dotnet/jit-contrib No spmi diffs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks Good
@jakobbotsch by my accounting this fixes all the test cases you sent me except for these 3:
The first two look like cases that should be handled by my fixes, so I will need to dig into them to see what is going wrong. In both cases the write/read sequence happens after a nested loop. |
If a loop is removed (because of unrolling) then the loop dependence tracking introduced in dotnet#55936 and dotnet#56184 may not properly update. So when a loop is removed, walk up the chain of parent loops looking for one that is not removed, and record the dependence on that parent. Addresses last part of dotnet#54118.
…56436) If a loop is removed (because of unrolling) then the loop dependence tracking introduced in #55936 and #56184 may not properly update. So when a loop is removed, walk up the chain of parent loops looking for one that is not removed, and record the dependence on that parent. Addresses last part of #54118.
The assert in that other test:
|
Looks like a flow opts bug, we end up not updating the pred list for BB08 properly and this ultimately leaves a dangling reference from BB07 to BB08 which causes the assert above.
|
The bug is here: runtime/src/coreclr/jit/fgopt.cpp Lines 5620 to 5629 in eb1e55e
This is new code that I added, I'll open a fresh issue for this... |
Specify
Overwrite
when setting loop dependence map entries, as we mayrefine the initial result.
Fixes #56174.
Extract loop dependence of
VNF_PhiMemoryDef
.Fixes new case noted in #55936, and 13/16 or so other cases Jakob sent
me privately. Also update a comment and fix tests to work better with
jitstress per other notes on that PR.