Annotate platform specific hardware intrinsics with the ConstExpected attribute#80192
Annotate platform specific hardware intrinsics with the ConstExpected attribute#80192tannergooding merged 3 commits intodotnet:mainfrom
Conversation
|
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
|
Tagging subscribers to this area: @dotnet/area-system-runtime-intrinsics Issue Detailsnull
|
| @@ -51,25 +51,25 @@ public abstract class AdvSimd : ArmBase | |||
| // /// float32x2_t vmla_lane_f32 (float32x2_t a, float32x2_t b, float32x2_t v, const int lane) | |||
| // /// A32: VMLA.F32 Dd, Dn, Dm[lane] | |||
| // /// </summary> | |||
| // public static Vector64<float> MultiplyAddBySelectedScalar(Vector64<float> addend, Vector64<float> left, Vector64<float> right, byte rightIndex) => MultiplyAddBySelectedScalar(addend, left, right, rightIndex); | |||
| // public static Vector64<float> MultiplyAddBySelectedScalar(Vector64<float> addend, Vector64<float> left, Vector64<float> right, [ConstantExpected(Max = (byte)(1))] byte rightIndex) => MultiplyAddBySelectedScalar(addend, left, right, rightIndex); | |||
There was a problem hiding this comment.
Arm64 and WASM expect values to be exactly in range. As such, we strictly limit the input between 0 and Max or 1 and Max.
The exact semantics is dependent on the instruction and is table driven in the JIT: https://github.com/dotnet/runtime/blob/main/src/coreclr/jit/hwintrinsicarm64.cpp#L215-L271
| /// VEXTRACTF128 xmm/m128, ymm, imm8 | ||
| /// </summary> | ||
| public static Vector128<byte> ExtractVector128(Vector256<byte> value, byte index) => ExtractVector128(value, index); | ||
| public static Vector128<byte> ExtractVector128(Vector256<byte> value, [ConstantExpected] byte index) => ExtractVector128(value, index); |
There was a problem hiding this comment.
x86/x64 on the other hand typically allows any input and will mask off to only the bits needed.
There are a couple exceptions, but it is also table driven on the JIT side: https://github.com/dotnet/runtime/blob/main/src/coreclr/jit/hwintrinsicxarch.cpp#L196-L230
| /// The scale parameter should be 1, 2, 4 or 8, otherwise, ArgumentOutOfRangeException will be thrown. | ||
| /// </summary> | ||
| public static unsafe Vector128<int> GatherVector128(int* baseAddress, Vector128<int> index, byte scale) | ||
| public static unsafe Vector128<int> GatherVector128(int* baseAddress, Vector128<int> index, [ConstantExpected(Min = (byte)(1), Max = (byte)(8))] byte scale) |
There was a problem hiding this comment.
The one particularly interesting case for x86/x64 is GatherVector and GatherMaskVector. These take exactly 1, 2, 4, or 8 with other values being disallowed.
This is most closely modeled on the attribute by restricting to between 1 and 8 inclusive.
eef2c1d to
3bb915a
Compare
dakersnar
left a comment
There was a problem hiding this comment.
Were these annotations all manually added? What were your source of truth for the expected constants in each annotation? How (if at all) are these tested?
I wrote a small program that added them.
The existing JIT conditions, which were originally seeded from the Architecture Manuals
Manual validation in VS that the analyzer gets triggered |
dakersnar
left a comment
There was a problem hiding this comment.
Thanks for the clarifications, this LGTM at a high level.
|
This seems to be breaking some wasm builds - https://dev.azure.com/dnceng-public/public/_build/results?buildId=127536&view=logs&jobId=63c2d0c8-fec2-5788-81c8-f3ac95e8841f&j=1edeb425-f545-5bac-61ef-500e115ab70f&t=34633226-1964-5e2b-9141-b7baebe1e47c . The changes in #80271 are unrelated as they come into effect when running tests. |
|
The fix is easy, but I'm unsure why it didn't trigger these failures in the PR. |
|
Fix is here: #80282 |
The xplat intrinsics do not need this as they use alternative instruction sequences and handling where possible. They do not need to fallback to anything like a jump table.