Extend System.Runtime.Intrinsics.Arm to support nint and nuint (ArmBase + Crc32)#127327
Conversation
|
@copilot please go with option C. But note that in the full task list that the |
… type to var_types Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/244a0b75-c1b0-46b3-b53e-23ed8cf7401a Co-authored-by: tannergooding <10487869+tannergooding@users.noreply.github.com>
Done in 6f34511 — landed the
|
There was a problem hiding this comment.
Pull request overview
This PR implements the approved System.Runtime.Intrinsics.Arm API expansion to add nint/nuint overloads for a small, scoped subset (ArmBase + Crc32), and refactors the JIT’s GenTreeHWIntrinsic “auxiliary type” tracking from CorInfoType to var_types to better match existing JIT type plumbing.
Changes:
- Added
nint/nuintoverloads toArmBase(LeadingZeroCount,ReverseElementBits) andArmBase.Arm64(LeadingSignCount), plusnuintoverloads toCrc32(ComputeCrc32,ComputeCrc32C). - Refactored
GenTreeHWIntrinsicauxiliary type storage/accessors to usevar_types, updating all impacted JIT call sites and removing now-deadCorInfoTypeSIMD base-type helpers. - Extended the ARM intrinsic test generator inputs to cover the new
ArmBasenint/nuintoverloads with pointer-size-aware validation.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tests/Common/GenerateHWIntrinsicTests/Arm/BaseTests.cs | Adds generator entries for ArmBase nint/nuint overload coverage. |
| src/libraries/System.Runtime.Intrinsics/ref/System.Runtime.Intrinsics.cs | Updates ref assembly with the new nint/nuint overload signatures. |
| src/libraries/System.Private.CoreLib/src/System/Runtime/Intrinsics/Arm/ArmBase.cs | Adds nint/nuint overloads to the supported implementation surface. |
| src/libraries/System.Private.CoreLib/src/System/Runtime/Intrinsics/Arm/ArmBase.PlatformNotSupported.cs | Mirrors new ArmBase overloads for unsupported platforms. |
| src/libraries/System.Private.CoreLib/src/System/Runtime/Intrinsics/Arm/Crc32.cs | Adds nuint overloads to Crc32 supported implementation surface. |
| src/libraries/System.Private.CoreLib/src/System/Runtime/Intrinsics/Arm/Crc32.PlatformNotSupported.cs | Mirrors new Crc32 overloads for unsupported platforms. |
| src/coreclr/jit/gentree.h | Changes auxiliary type storage to var_types (gtAuxiliaryType) and updates accessors. |
| src/coreclr/jit/gentree.cpp | Updates cloning/folding/memory-load logic to use var_types auxiliary type APIs. |
| src/coreclr/jit/hwintrinsic.cpp | Migrates auxiliary type tracking to var_types and updates affected import paths. |
| src/coreclr/jit/hwintrinsicarm64.cpp | Migrates auxiliary type tracking to var_types for Arm64 intrinsics import paths. |
| src/coreclr/jit/hwintrinsicxarch.cpp | Migrates auxiliary type tracking to var_types for xarch intrinsic import paths. |
| src/coreclr/jit/valuenum.cpp | Migrates auxiliary type checks to var_types for ARM64 constant folding/value numbering. |
| src/coreclr/jit/simd.cpp | Removes the CorInfoType-returning SIMD base-type helper (now unused). |
| src/coreclr/jit/compiler.h | Removes declarations for the deleted CorInfoType SIMD base-type helpers. |
… slots for nint/nuint dispatch Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/c2ab4e66-3233-4d8d-b691-668aa5db1675 Co-authored-by: tannergooding <10487869+tannergooding@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
src/libraries/System.Runtime.Intrinsics/ref/System.Runtime.Intrinsics.cs:1
- The overload ordering for
ComputeCrc32*is now inconsistent (e.g.,ushortappears afteruint, whilebyte/nuint/uintare earlier). For ref assemblies this impacts how APIs are presented in diffs/reviews and can affect tooling that expects stable ordering. Reorder overloads consistently (commonly by increasing operand size:byte,ushort,uint,nuint/ulongas applicable) for bothComputeCrc32andComputeCrc32C.
🤖 Copilot Code Review — PR #127327Note This review was generated by GitHub Copilot. Holistic AssessmentMotivation: Justified and well-scoped. The PR implements a subset of the approved API proposal (#52027, Approach: Sound. Storing Summary: ✅ LGTM. The refactor is semantically correct across all ~18 migrated call sites, the new APIs match the approved proposal exactly, and the HW intrinsic list entries are correct. One minor pre-existing typo is noted below. The PR has already been approved by Detailed Findings✅ API Approval Verification — Approved via #52027Issue #52027 has the
The ref assembly, ✅ Auxiliary Type Refactor — Semantically correctThe
✅ StoreNarrowing ptrType Initialization — Dead store removed correctlyIn ✅ Dead Code Removal —
|
…ors with T.LeadingZeroCount Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/d634db26-845d-4344-8cb7-00794c9dd590 Co-authored-by: tannergooding <10487869+tannergooding@users.noreply.github.com>
…Crc32C_UIntPtr) Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/d9436184-20aa-448e-87fa-61358beca1c7 Co-authored-by: tannergooding <10487869+tannergooding@users.noreply.github.com>
Implements the approved API proposal to extend
System.Runtime.Intrinsics.Armwithnint/nuintoverloads. Per reviewer guidance, this PR is scoped down to Option C: theArmBase+Crc32managed surface plus theGenTreeHWIntrinsicauxiliary-type refactor. The bulk of the proposal (AdvSimd+AdvSimd.Arm64) is deferred to follow-up PRs.Description
Managed surface (implementation,
PlatformNotSupportedmirror, and ref assembly)ArmBaseLeadingZeroCount(nint),LeadingZeroCount(nuint)ReverseElementBits(nint),ReverseElementBits(nuint)ArmBase.Arm64LeadingSignCount(nint)Crc32ComputeCrc32(uint crc, nuint data)ComputeCrc32C(uint crc, nuint data)No APIs are added beyond the approved proposal. The managed bodies are simple self-call intrinsic stubs; the JIT handles dispatch via the hwintrinsic list (see below).
JIT hwintrinsic list — 64-bit instruction slots for the 32-bit classes
In
src/coreclr/jit/hwintrinsiclistarm64.h, theTYP_LONG/TYP_ULONGslots of the 32-bit-class entries are now populated so thatnint/nuintoverloads (whichJitType2PreciseVarTypemaps toTYP_LONG/TYP_ULONGon 64-bit) dispatch to the correct Arm64 instruction without needing a managed-side redirect to*.Arm64.*:NI_ArmBase_LeadingZeroCount— addedINS_clzforTYP_LONG/TYP_ULONGNI_ArmBase_ReverseElementBits— addedINS_rbitforTYP_LONG/TYP_ULONGNI_Crc32_ComputeCrc32— addedINS_crc32xforTYP_ULONGNI_Crc32_ComputeCrc32C— addedINS_crc32cxforTYP_ULONGCodegen uses
emitActualTypeSize(intrin.baseType)forHW_Category_Scalar, so the 8-byte operand size flows through automatically. On Arm32,nint/nuintis 32-bit and theseTYP_LONG/TYP_ULONGslots are never reached.JIT refactor —
GenTreeHWIntrinsicauxiliary typeSwitched the tracked auxiliary type from
CorInfoTypetovar_types:gtAuxiliaryJitType→gtAuxiliaryType, default changed fromCORINFO_TYPE_UNDEFtoTYP_UNKNOWN.GetAuxiliaryJitType()/SetAuxiliaryJitType(CorInfoType)removed.GetAuxiliaryType()now returns the storedvar_typesdirectly; newSetAuxiliaryType(var_types)added.hwintrinsic.cpp,hwintrinsicarm64.cpp,hwintrinsicxarch.cpp,gentree.cpp, andvaluenum.cpp. SentinelsCORINFO_TYPE_PTR/CORINFO_TYPE_UNDEFare translated at the boundary toTYP_U_IMPL/TYP_UNKNOWN.getBaseJitTypeAndSizeOfSIMDType/getBaseJitTypeOfSIMDTypehelpers are removed fromcompiler.handsimd.cpp; callers use the existingvar_types-returninggetBaseTypeAndSizeOfSIMDType/getBaseTypeOfSIMDType.Tests
New generator inputs added in
src/tests/Common/GenerateHWIntrinsicTests/Arm/BaseTests.csfor thenint/nuintoverloads ofLeadingZeroCount,LeadingSignCount,ReverseElementBits,ComputeCrc32, andComputeCrc32C.While there, the bit-by-bit
for-loop expected-result computations for allLeadingZeroCount_*andLeadingSignCount_*validators (pre-existing entries plus the new ones) were rewritten to use the BCLT.LeadingZeroCountAPI directly. This fixes a pre-existing non-termination bug for edge inputs (data == 0for LZC,0or-1for LSC) where the loop counter would go negative without ever exiting.LeadingZeroCount_*:int expectedResult = (int)T.LeadingZeroCount(data);(withTbeingint/uint/long/ulong/nint/nuint).LeadingSignCount_*:int expectedResult = (int)T.LeadingZeroCount(data ^ (data >> (N-1))) - 1;— an arithmetic-shift sign-extension XOR that matches ArmCLSsemantics for all inputs (verified for0→N-1,-1→N-1,int.MinValue→0, etc.).For the new
Crc32nuintoverloads (ComputeCrc32_UIntPtr,ComputeCrc32C_UIntPtr), the validator compares against the explicit-typed ground-truth overloads via a JIT-constant branch onUIntPtr.Size:This gives end-to-end coverage on both Arm32 (CRC32W via the
uintoverload) and Arm64 (CRC32X via theArm64.ulongoverload), confirms the newnuintoverload dispatches to the correct instruction width, and catches any self-recursion or wrong-width lowering.Scope deferred to follow-up PRs
AdvSimdandAdvSimd.Arm64nint/nuintoverloads (hundreds of methods inAdvSimd.cs/AdvSimd.PlatformNotSupported.cs).AdvSimdoverloads.Testing
./build.sh clr+libs -rc release— 0 errors, 0 warnings. JIT C++ refactor compiles clean on x64; the managed surface (including the ref assembly) compiles clean../build.sh clr.alljits -c release— cross-compiles all JIT targets includinglibclrjit_universal_arm64_x64.so, exercising the updatedhwintrinsiclistarm64.h. 0 errors, 0 warnings.GenerateHWIntrinsicTests_Armbuilds clean and emits the expected validator code for all new tests (LeadingZeroCount_*,LeadingSignCount_*,ReverseElementBits_*,ComputeCrc32_UIntPtr,ComputeCrc32C_UIntPtr) across Int32/UInt32/Int64/UInt64/IntPtr/UIntPtr variants.