Add AvxIfma + AvxIfma.V512 hardware intrinsics - #130079
Conversation
Adds the managed AvxIfma class (V128/V256 VEX form) and nested AvxIfma.V512 (EVEX form) for VPMADD52LUQ / VPMADD52HUQ. Structural changes only (no tests yet, no build validation yet): - hwintrinsiclistxarch.h: new AVXIFMA range + AVX512v2 IFMA entries - hwintrinsicxarch.cpp: V512VersionOfIsa AVXIFMA/AVX512v2 case, lookupInstructionSet Ifma dispatch - hwintrinsiccodegenxarch.cpp: dispatch cases for MultiplyAdd52Low/High - lsraxarch.cpp: RMW cases - lowerxarch.cpp: default-branch assert extension - CorInfoInstructionSet.cs: nested type lookup + LookupPlatformIntrinsicTypes - InstructionSetDesc.txt: nested type alias AvxIfma_V512 - InstructionSetHelpers.cs: optimistic support - projitems: include new files - AvxIfma.cs + AvxIfma.PlatformNotSupported.cs: managed surface - System.Runtime.Intrinsics.cs: ref assembly Closes dotnet#98833, closes dotnet#96476.
Adds sample tests mirroring the AvxVnni.V512 test shape: - X86_Avx/AvxIfma/ — V128/V256 VEX form - X86_Avx/AvxIfma_V512/ — V512 EVEX form NativeAOT smoke test: uncomments AvxIfma / AvxIfma.X64 checks (were pre-approved but not yet wired) and adds AvxIfma.V512 check. AvxIfma.V512 folds under AVX512v2 (Avx512Vbmi is a representative sibling).
github-actions
Bot
added
the
area-CodeGen-coreclr
dotnet-policy-service
Bot
added
the
community-contribution
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
There was a problem hiding this comment.
Pull request overview
This PR adds the managed System.Runtime.Intrinsics.X86.AvxIfma surface (VEX AVX-IFMA for Vector128/256<ulong>) plus a nested AvxIfma.V512 surface (EVEX AVX-512-IFMA for Vector512<ulong>), and wires the corresponding JIT/AOT instruction-set plumbing and sample tests.
Changes:
- Add
AvxIfma/AvxIfma.V512public API surface (ref + CoreLib implementation + PNSE stubs) and include it in CoreLib build items. - Extend JIT HWIntrinsic tables and codegen/LSRA/lowering handling for
VPMADD52LUQ/HUQ(VEX + EVEX forms). - Add JIT and NativeAOT smoke tests covering the new intrinsics.
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/System.Private.CoreLib/src/System/Runtime/Intrinsics/X86/AvxIfma.cs | Adds managed intrinsic declarations for AVX-IFMA + nested V512 surface. |
| src/libraries/System.Private.CoreLib/src/System/Runtime/Intrinsics/X86/AvxIfma.PlatformNotSupported.cs | Adds PNSE stubs for unsupported platforms/hardware. |
| src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems | Wires new CoreLib source files into the build. |
| src/libraries/System.Runtime.Intrinsics/ref/System.Runtime.Intrinsics.cs | Adds public ref-assembly API surface for AvxIfma / AvxIfma.V512. |
| src/coreclr/jit/hwintrinsicxarch.cpp | Updates ISA lookup / V512 ISA mapping for the new intrinsics. |
| src/coreclr/jit/hwintrinsiclistxarch.h | Adds AVXIFMA and AVX512v2 intrinsic table entries for MultiplyAdd52*. |
| src/coreclr/jit/hwintrinsiccodegenxarch.cpp | Adds codegen dispatch coverage for the new intrinsic IDs. |
| src/coreclr/jit/lsraxarch.cpp | Adds LSRA handling for the new RMW-style 3-operand intrinsics. |
| src/coreclr/jit/lowerxarch.cpp | Extends containment asserts/logic to include the new intrinsic IDs. |
| src/coreclr/tools/Common/JitInterface/CorInfoInstructionSet.cs | Adds type/ISA mapping entries for nested AvxIfma forms and AVX512v2 aliasing. |
| src/coreclr/tools/Common/JitInterface/ThunkGenerator/InstructionSetDesc.txt | Adds AvxIfma_V512 alias mapping for the Avx512Ifma row. |
| src/coreclr/tools/Common/InstructionSetHelpers.cs | Adds avxifma_v512 to the optimistic instruction-set set. |
| src/tests/JIT/HardwareIntrinsics/X86_Avx/AvxIfma/Program.AvxIfma.cs | Adds test program stub/namespace for AVXIFMA tests. |
| src/tests/JIT/HardwareIntrinsics/X86_Avx/AvxIfma/AvxIfmaSampleTest.cs | Adds V128/V256 semantic sample tests for MultiplyAdd52*. |
| src/tests/JIT/HardwareIntrinsics/X86_Avx/AvxIfma/*.csproj | Adds project files to run the AVXIFMA JIT tests in r/ro variants. |
| src/tests/JIT/HardwareIntrinsics/X86_Avx/AvxIfma_V512/Program.AvxIfma_V512.cs | Adds test program stub/namespace for AVXIFMA V512 tests. |
| src/tests/JIT/HardwareIntrinsics/X86_Avx/AvxIfma_V512/AvxIfma_V512SampleTest.cs | Adds V512 semantic sample tests for MultiplyAdd52*. |
| src/tests/JIT/HardwareIntrinsics/X86_Avx/AvxIfma_V512/*.csproj | Adds project files to run the AVXIFMA V512 JIT tests in r/ro variants. |
| src/tests/nativeaot/SmokeTests/HardwareIntrinsics/Program.cs | Enables NativeAOT smoke checks for AvxIfma and adds AvxIfma.V512. |
…tical binarySearchId uses strcmp (case-sensitive ASCII order) to find intrinsics within an ISA range. My initial ordering put MultiplyAdd52Low before MultiplyAdd52High, but 'H' (0x48) < 'L' (0x4C), so the binary search would fail on High. Same issue for both AVXIFMA and AVX512v2 ranges. Also reorders MultiplyAdd52*/PermuteVar* so Multiply* precedes PermuteVar* (M < P). This surfaced as ILC 'Code generation failed' on the AvxIfma_handwritten_r NativeAOT tests.
The LookupPlatformIntrinsicTypes switch was missing cases for InstructionSet.X64_AVXIFMA/AVXIFMA_X64 and X86_AVXIFMA. When the AOT compiler tried to resolve AvxIfma against an AVXIFMA-only target (e.g. NativeAOT smoke tests X64Baseline / X64Avx2), the type wasn't found and ILC failed with 'Code generation failed for AvxIfma.get_IsSupported()'. Mirrors the existing X64_AVXVNNI/X86_AVXVNNI cases.
InstructionSetDesc.txt had AvxIfma in the R2R-name column (col 4) instead of the managed-name column (col 3), so the generated ArchitectureToValidInstructionSets emitted 'avxifma' with an empty ManagedName. HardwareIntrinsicILProvider builds its class-name -> InstructionSet dictionary from ArchitectureToValidInstructionSets, skipping entries with empty ManagedName, so lookups for 'AvxIfma' threw KeyNotFoundException at ILC time. Fixed the .txt source-of-truth (so any regeneration is consistent) and the already-generated CorInfoInstructionSet.cs to set ManagedName='AvxIfma'.
|
The 4 current CI failures on this PR are pre-existing infra/flake issues unrelated to this change (verified via the Build Analysis check):
None touch x86/x64 codegen or the hardware-intrinsic dispatch paths this PR changes. Happy to retrigger if useful. |
# Conflicts: # src/coreclr/jit/hwintrinsiclistxarch.h
Per Tanner's review feedback: these entries are reachable via the AvxIfma class-name fallback (when AVXIFMA isn't supported but AVX512v2 is), so a fixed simdSize=64 causes the importer's lookupSimdSize short-circuit to hardcode the node's SIMD size to 64 even for genuine Vector128/256 call sites. Setting simdSize=-1 matches the AVXIFMA entries and lets the importer derive the actual width from the call site signature instead.
|
@tannergooding Just checking in — it's been a few days since your last review pass. The simdSize fix you flagged is in (52cbd74) and all review threads are resolved. CI remains clean aside from the pre-existing/triaged flakes noted above. Let me know if there's anything else you'd like me to address before this is ready to merge. |
|
@tannergooding Following up once more on the check-in above — it's now been about a week with no reply, and I want to make sure I'm not missing anything on my end. Is there anything blocking this that I should address (design concern, competing priority, something else entirely)? Happy to make any changes needed — just let me know what would help move it forward. |
|
Sorry, there's a lot of in flight PRs and work items to get through as part of locking down .NET 11 for RC1. This is on my radar, I just haven't had time to get to it yet. |
|
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "52cbd74cbe1bb8640c2abdbcf35fe0cad48ba2de",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "e4cdfeebacd6d77a0b63cda25e2a2f30ab040941",
"last_reviewed_commit": "52cbd74cbe1bb8640c2abdbcf35fe0cad48ba2de",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "e4cdfeebacd6d77a0b63cda25e2a2f30ab040941",
"last_recorded_worker_run_id": "29682993446",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "52cbd74cbe1bb8640c2abdbcf35fe0cad48ba2de",
"review_id": 4730591057
}
]
} |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Holistic Review
Motivation: Justified. The PR exposes the AVX-IFMA (VPMADD52LUQ/VPMADD52HUQ) intrinsics on Vector128/256/512<ulong>, closing two api-approved proposals (#98833 and #96476). Most of the JIT/runtime plumbing (instruction encodings, EVEX/VEX gating, R2R IDs, CPUID gates, config knob) already existed, so this is the small managed + wire-up layer needed to light it up.
Approach: Sound and idiomatic. It closely mirrors the recently merged AvxVnni / AvxVnni.V512 pattern (#128365): a nested AvxIfma.V512 folded under the AVX512v2 JIT ISA, class-name dispatch in lookupInstructionSet selecting AVXIFMA vs AVX512v2 by compSupportsHWIntrinsic, and V512VersionOfIsa mapping both to AVX512v2. The VEX entries carry HW_Flag_NoEvexSemantics and the four NIs are added to the same codegen/LSRA/lowering RMW dispatch groups as the analogous VNNI intrinsics.
Summary: ✅ LGTM. I verified the implemented public surface in ref/System.Runtime.Intrinsics.cs and AvxIfma.cs/AvxIfma.PlatformNotSupported.cs matches the approved shapes on #98833 (VEX V128/V256 + X64) and #96476 / saucecontrol's restructure (nested V512), including the addend, left, right parameter names terrajobst called out. Both linked issues carry the api-approved label. JIT wire-up, R2R/AOT metadata (CorInfoInstructionSet.cs, InstructionSetDesc.txt, InstructionSetHelpers.cs), and the nativeaot smoke test all follow the established VNNI precedent consistently. Tests cover supported/unsupported paths and validate 52-bit masking, high/low product, and accumulation semantics for all three widths. I could not build or run in this environment; correctness of the JIT dispatch and encoding selection should be confirmed by CI (HW intrinsics test legs), but no blocking issues were found by reading.
Detailed Findings
✅ API conformance — Matches approved proposals
The ref surface and both src implementations match the approved shapes for #98833 and #96476 exactly (type names, namespaces, method signatures, and parameter names addend/left/right). No extra or missing public surface.
✅ JIT / R2R wire-up — Consistent with AvxVnni.V512 precedent
lookupInstructionSet dispatch for Ifma, V512VersionOfIsa mapping AVXIFMA/AVX512v2→AVX512v2, the FIRST_NI_AVXIFMA/LAST_NI_AVXIFMA range, and the CorInfoInstructionSet.cs nested-type lookups (AvxIfma → X64/V512/V512_X64) all follow the AvxVnni model landed in #128365. InstructionSetDesc.txt correctly moves the AvxIfma/Avx512Ifma aliases into the managed-name columns.
✅ Tests — Good semantic coverage
The JIT sample tests and nativeaot smoke test exercise both the supported and PlatformNotSupported paths and validate the IFMA semantics (low/high 52-bit product, 52-bit source masking, addend accumulation, and lane-varying patterns). The AvxIfma.V512 smoke check correctly reuses ExpectedAvx512Vbmi (a representative AVX512v2 sibling), which is defined in every preprocessor configuration branch, so it compiles across all legs.
💡 Observation — V512_X64 metadata without public surface
CorInfoInstructionSet.cs handles a V512_X64 nested type for AvxIfma, but there is no AvxIfma.V512.X64 in the approved API or the ref surface (only AvxIfma.X64). This mirrors the AvxVnni handling and is harmless (the lookups guard on GetNestedType returning non-null), but the V512_X64 branch is effectively dead for this type. Non-blocking; noted only for awareness.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 201.6 AIC · ⌖ 10.8 AIC · ⊞ 10K
Summary
Adds the managed
AvxIfmasurface for the AVX-IFMA (VEX) and AVX-512-IFMA (EVEX) ISAs, exposingVPMADD52LUQ/VPMADD52HUQonVector128<ulong>/Vector256<ulong>/Vector512<ulong>.Closes #98833 (AVX-IFMA — VEX, V128/V256)
Closes #96476 (AVX-512-IFMA — EVEX, V512)
Both handled together per Tanner's direction on #98833:
Shape
Mirrors the
AvxVnni/AvxVnni.V512pattern that landed in #128365, and the shape @saucecontrol pre-proposed for IFMA on 2024-11-06:AvxIfma.V512folds under the existingAVX512v2JIT instruction set (same relationship asAvxVnni.V512→AVX512v3in #128365). The class-name dispatch inlookupInstructionSetchoosesAVXIFMAwhen that CPUID bit is present, otherwiseAVX512v2— so on a hypothetical machine with AVX-512-IFMA but not VEX AVX-IFMA,AvxIfma.V512.IsSupported(andAvxIfma.IsSupported) still reporttrue.What was already wired
Significant pre-existing plumbing meant this PR is small:
INS_vpmadd52luq/INS_vpmadd52huqalready ininstrsxarch.h(FIRST_AVXIFMA_INSTRUCTION..LAST_AVXIFMA_INSTRUCTION)emitxarch.cppalready gates VEX form onInstructionSet_AVXIFMA(line 353) and EVEX form onInstructionSet_AVX512v2(line 430)Is3OpRmwInstructionalready returnstruevia the AVXIFMA range checkInstructionSet_AVXIFMA/AVXIFMA_X64enums, R2R IDs (AvxIfma=66,Avx512Ifma=76),EnableAVXIFMAconfig, andcodeman.cppCPUID gate — all presentWhat this PR adds
JIT wire-up:
hwintrinsiclistxarch.h: newAVXIFMArange (MultiplyAdd52Low/MultiplyAdd52Highat ULONG slot withHW_Flag_NoEvexSemantics) + AVX512v2 V512 entrieshwintrinsicxarch.cpp:V512VersionOfIsamaps AVXIFMA/AVX512v2 → AVX512v2;lookupInstructionSetdispatches "Ifma" class between AVXIFMA and AVX512v2 bycompSupportsHWIntrinsichwintrinsiccodegenxarch.cpp: dispatch cases for the 4 new NIs alongside the existing VNNI/AVX512v3 grouplsraxarch.cpp: RMW handling for the 4 new NIslowerxarch.cpp: default-branch assert extended for the new NIs + AVXIFMA rangeR2R + AOT metadata:
CorInfoInstructionSet.cs: nested-type lookup forAvxIfma(X64/V512/V512_X64) on X64 and X86 +LookupPlatformIntrinsicTypesfor the AVX512v2 case + newInstructionSetInfo("avx512v2", "AvxIfma_V512", ...)entriesInstructionSetDesc.txt: addsAvxIfma_V512alias to the Avx512Ifma rowInstructionSetHelpers.cs: addsavxifma_v512to the optimistic setManaged surface:
AvxIfma.cs+AvxIfma.PlatformNotSupported.cs(new files).Shared.projitems: includes bothSystem.Runtime.Intrinsics.cs(ref assembly): full API surfaceTests:
X86_Avx/AvxIfma/— V128/V256 VEX form sample testX86_Avx/AvxIfma_V512/— V512 EVEX form sample testAvxIfma/AvxIfma.X64checks (were pre-approved stubs) and addsAvxIfma.V512checkValidation
build.cmd clr.jit+clr.runtime+libs -c Release -arch x64— 0 errors / 0 warningsifmaprobe/ probe DLL swap deferred to CI: locally the packs pipeline is currently broken by a WiX installer failure, so the standardRestoreAdditionalProjectSources → Shippingapproach doesn't refresh. Runtime verification will land via the CI test legs.Compatibility note
The R2R ID
Avx512Ifma = 76retains its numeric value; existing R2R images continue to work. TheInstructionSetDesc.txtrow now recordsAvxIfma_V512as the nested-type alias, matching the pattern PR #128365 used forAvx512Vnni→AvxVnni_V512.🤖 Generated with Claude Code