-
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 JIT\\HardwareIntrinsics\\General\\Vector256\\Vector256_ro\\Vector256_ro.cmd #70642
Comments
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsRun: runtime-coreclr jitstress2-jitstressregs 20220612.1 Failed test:
Error message:
|
I have a possible repro with: COMPlus_JitStress=2
COMPlus_JitStressRegs=1
COMPlus_TieredCompilation=0 where
|
Failed again in: runtime-coreclr jitstress2-jitstressregs 20220618.1 Failed test:
Error message:
|
Failed again in: runtime-coreclr jitstress2-jitstressregs 20220626.1 Failed test:
Error message:
|
Failed again in: runtime-coreclr jitstress2-jitstressregs 20220710.1 Failed test:
Error message:
|
@tannergooding, this test is constantly failing. Could you take a look? |
Failed again in: runtime-coreclr jitstress2-jitstressregs 20220724.1 Failed Test:
Error Message
|
Failed again in: runtime-coreclr jitstress2-jitstressregs 20220814.1 Failed Test:
Error message:
|
@tannergooding, it failed again yesterday. Please check this out. |
@kunalspathak Could you take a look at this? |
Sure. |
The failure case is as @tannergooding mentioned above: #70642 (comment) With this particular stress setup, some of the test cases are inlined into the "main" function In the inlined predecessor, we have
In the inlined
but, as seen, because it is marked as The bug is, there was a call between definition of The intermediate call doesn't have any specific kill of
Looks like there's some missing logic for ReuseReg and the FEATURE_PARTIAL_SIMD_CALLEE_SAVE case. |
Introduced by #70171. Still investigating. |
@BruceForstall and I spoke offline and as he pointed, there is a case missing where we need to either kill the upper-half of vector register if it was callee-save or save/restore if it was callee-thrash. We create ref positions during building the interval, but we won't know which register got assigned until we come to the register assignment. I will think little bit more and try out some ideas that Bruce suggested. |
@kunalspathak Given that the fix will probably require significant new work, I think we should revert #70171 and port the revert to .NET 7. A new implementation with fix can be made in .NET 8. |
Yes, I was thinking on similar lines given that #70171 should be straightforward to revert. I will try few things for couple of hours and if they don't work, will submit a revert PR. |
I expect the "right" fix might be something involving #70182. For these types of constants, it is cheaper to reinitialize it entirely than to involve it with caller/callee save.
In both cases, avoiding spilling and opting to reconstitute the value directly will end up being "better" for the cases where a spill does happen. |
Probably. As you know, currently, the problem is with #70171, we are doing opposite of reinitializing the constant, and instead trying to "reuse" it. In "reuse" case, we only save/restore upper half if it holds a live variable (and not constant as far as I can tell). I will see if we can patch the save/restore upper half criteria to include it. We might not able to piggyback on the kill refpositions to clear the constants (as I was discussing with Bruce) because the kill positions are per register and in our case, there is no kill refposition inserted for upper-half vector register. |
Hhhm, I cannot do that because the decision of saving/restore upper-half happens during interval building, while we won't know which register holds a constant (and hence need to be saved/restored) until register assignment. |
Run: runtime-coreclr jitstress2-jitstressregs 20220612.1
Failed test:
Error message:
The text was updated successfully, but these errors were encountered: