-
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
Fix various asserts that were found by Antigen #104625
Conversation
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
case INS_vbroadcastf32x8: | ||
case INS_vbroadcasti32x8: | ||
case INS_vbroadcasti64x4: | ||
case INS_vbroadcastf64x4: |
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.
These return TYP_SIMD64 but take in TYP_SIMD32 and so we generated wrong disassembly and asserted when validating the alignment of the memory operand.
CC. @dotnet/jit-contrib This should be ready for review and fixes a few asserts that were caught by antigen. SPMI replay failure is #104585 |
Rerunning now that the SPMI replay issue should be fixed. |
Can we add some test cases, if they were just caught by Antigen but not by P0/P1 test suite? |
It was explicitly commented at the top that most of these were not something for which simplified reproduction could be identified. They often repro because the antigen source generated is massive and impacts the JIT in weird ways, particularly in how some optimizations/transforms may light up for parts of a method, but not the rest. For most of them, the JIT never gets to an incorrect sequence otherwise because everything else was already correct or would be coerced to be correct under the default transforms |
Happy for tests to be added if someone knows a way to coerce the JIT to do the right partial transforms here, but I had already spent a couple hours playing around with C# that should trigger it but wouldn’t, even with some of the config knobs that do things like disable vn or cse to try and get it into the right shape for codegen/lowering |
These were found by antigen (https://dev.azure.com/dnceng-public/public/_build/results?buildId=732346&view=ms.vss-build-web.run-extensions-tab), however the test cases are massive and often not trivially simplified so there isn't an easy way to add regression coverage here.