-
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
Test failure: XDocumentTests.Streaming.XStreamingElementAPI.NestedXStreamingElementPlusIEnumerable #76636
Comments
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsJitStress=1 pipeline: runtime-coreclr libraries-jitstress
Passes without JitStress (or with JitStress=2). @dotnet/jit-contrib
|
Similar failure in
|
I'll take a look. |
For the first failure above, on arm64 windows at 144a33a,
(there are possibly other methods that also lead to failures when stressed). |
Fails with
There is a phi-based RBO. Disabling this opt fixes the problem. Debugging now to see if this transformation is wrong or enables something else downstream to go wrong.
|
Optimization seems like it is correct: in
|
Looks like this is an issue where phi-based RBO leaves an invalid SSA graph that trips up assertion prop. Pre RBO we have VN is able to prove that Note that Assertion prop comes along and is now able to deduce that I don't see an easy fix here yet. Phi-based RBO either needs to properly update SSA (which will be tricky given that dominators are now wrong, and we do not track where to find an SSA def's uses) or else avoid making changes in cases where the bypassed PHI value ( Given this issue and #76507 I am going to disable phi-based RBO for now while I think about whether it can be salvaged somehow. |
This is exposing our lack of SSA update and leading downstream opts like CSE and assertion prop to make bad decisions. Disabling for now until I have time to figure out how to safely enable. Fixes dotnet#76636, dotnet#76507
Presumably we could re-build SSA/VN after RBO (if necessary)? Also presumably that would be too expensive for JITing, but maybe ok for AOT? |
Are there any post-phase asserts we could add for the SSA form, especially asserts that might have prevented this issue? E.g., simplistically, can we assert that the PHI arity must match the count of immediate predecessors? |
SSA does not give us one phi arg per pred; it has one phi arg per reaching SSA def (see eg Even nicer perhaps would be to have the phi arg ordering reflect the pred list ordering. But this might be too painful to maintain if/when we decide to support some form of SSA update. |
It seems like there are at least a few things relating preds and PHI args, though. E.g.,
? |
I see
If phi args aren't 1-to-1 with preds then it seems like ordering is less interesting? |
If the phi args name their preds (like they sort of do now) then ordering is not important. Let me think about what sort of SSA checks we can plausibly do. I suspect we'll find that we very quickly do damage, and it will seem even more incautious to keep leveraging SSA the way we do now. However perhaps we can track enough to allow a more limited form of the phi-disambiguation to be turned on (if we can show that no SSA update is needed). The main thing we need to know is if all the uses of a phi def are in the same block as the phi def. |
JitStress=1
pipeline: runtime-coreclr libraries-jitstress
https://dev.azure.com/dnceng-public/public/_build/results?buildId=39575&view=ms.vss-test-web.build-test-results-tab
Passes without JitStress (or with JitStress=2).
@dotnet/jit-contrib
The text was updated successfully, but these errors were encountered: