-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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] ARM64 - Do not allow move and shifting with MSL on 16-bit vectors #77123
Conversation
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsDecription Should resolve: #76880 We were allowing the Acceptance Criteria
|
Cool, thanks! What code do we generate instead? |
We generate:
Which basically does an inverse of the bits if I'm reading this correctly: https://developer.arm.com/documentation/ddi0596/2021-12/SIMD-FP-Instructions/MVNI--Move-inverted-Immediate--vector--?lang=en This is determined in the emitter from this logic: // Next try the ones-complement form of the 'immediate' imm(i8,bySh)
if ((elemsize == EA_2BYTE) || (elemsize == EA_4BYTE)) // Only EA_2BYTE or EA_4BYTE forms
{
notOfImm = NOT_helper(imm, getBitWidth(elemsize));
canEncode = canEncodeByteShiftedImm(notOfImm, elemsize, true, &bsi);
if (canEncode)
{
imm = bsi.immBSVal;
ins = INS_mvni; // uses a mvni encoding
assert(isValidImmBSVal(imm, size));
fmt = IF_DV_1B;
break;
}
} |
Wonder if something bad happened to our azure storage:
Similar problems in the SPMI legs. @BruceForstall any ideas? |
The Azure Storage issue should be fixed now. Please re-run the failing jobs. |
@TIHan can this happen in .NET 7 or is this something new? Wondering if we might need to backport. |
@AndyAyersMS It looks like it could happen in .NET 7. Some of the code surrounding this hasn't changed since 2015. |
The simple repro you created in #76880 works ok on .NET 7 RC1, so perhaps this is a recent change? Also, the test that fails here probably hasn't been failing for long. Might be good to pin down when our codegen changed.
[Edit: changed to show minopts codegen] |
The disasm you posted is optimized. I think it only emits movi when it is not. |
True -- fixed it above. Still doesn't have the bug. |
I'll try to investigate when this started occurring then. |
@dotnet/jit-contrib any way to get past this without starting from scratch?
|
You can ignore it. This happens we using "Rerun" to re-run a portion of a job. We use the AzDO There is some discussion on this here, for example: https://github.com/MicrosoftDocs/azure-devops-docs/issues/10543 |
For logs, using $(System.JobAttempt) is probably a good fit. However, I don't understand why the docs say "Available in templates? -- no", so it might take some variable shenanigans to get the value in. |
Just want to know if this PR had any diffs. It looks like it doesn't. @TIHan did you do a local SPMI diff run? |
I'll run a local SuperPMI diff to see what we get. |
There are no SuperPMI diffs found:
|
/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress |
Azure Pipelines successfully started running 2 pipeline(s). |
Co-authored-by: Bruce Forstall <[email protected]>
Decription
Should resolve: #76880
We were allowing the
movi
instruction in conjunction withMSL
to be emitted on 16-bit vectors which is not supported; it is only supported for 32-bit vectors. See https://developer.arm.com/documentation/ddi0596/2021-12/SIMD-FP-Instructions/MOVI--Move-Immediate--vector--?lang=enAcceptance Criteria