Repository navigation
[rocjitsu] Implement gfx1251 F64 WMMA execution. - #11124
Conversation
26e80e9 to
8ed5a3b
Compare
31241ef to
1aae284
Compare
newling
left a comment
newling
left a comment
There was a problem hiding this comment.
LGTM. I have an AI review here: https://github.com/newling/agent-public-reviews/blob/main/rocm-systems/review_pr11124.md
The suggestions there should be ignored (the first is about variable names, the second is about a test that I don't think is necessary).
I also discussed the relationship of some emulation performance work I recently did for mfma/wmma, to this PR. It seems like this PR does most of the 'right' things, but there might be some follow-up work for SIMD, definitely not needed in this PR.
atgutier
left a comment
atgutier
left a comment
There was a problem hiding this comment.
Agent found a few issues:
[MAJOR] emulation/rocjitsu/lib/python/amdisa/codegen/_generator.py:10377 — Legal F64 WMMA inline accumulators are rejected
The generated validator accepts src2 only when it is selector 242 (1.0) or an aligned 16-register VGPR tuple. This rejects other legal inline accumulators.
Evidence:
-
LLVM defines this operand as ROCm llvm-project/llvm/lib/Target/AMDGPU/SIRegisterInfo.td:1578, a VGPR-or-FP64-inline operand.
-
llvm-mc from the exact referenced LLVM tree accepts and encodes 0, 0.5, -1.0, integer inline constants, and the other standard FP64 inline values.
-
rocjitsu’s generated operand definitions likewise list these selectors in emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/isa/arch/amdgpu/generated/cdna5/operand_types.h:295.
-
The new test incorrectly codifies selector 243 (-1.0) as invalid at emulation/rocjitsu/tests/cdna5_execution_test.cpp:1699.
Valid compiler-produced kernels—particularly zero-initialized WMMA accumulations—can therefore fail during decode.
Suggested fix: accept the complete legal inline-selector set and apply tuple span/alignment
validation only when src2 is a VGPR. Add decode/execution coverage for at least 0, 0.5, and -1.0.
Confidence: high.
[MAJOR] emulation/rocjitsu/lib/python/amdisa/codegen/execute/matrix.py:424 — Execution is made MODE-dependent despite LLVM explicitly marking WMMA as MODE-independent
The generated callback passes the wavefront’s F64 rounding and denormal modes into every WMMA reduction. The new tests then require results to vary when those MODE fields change.
However, LLVM defines all GFX12 WMMA instructions with ROCm llvm-project/llvm/lib/Target/AMDGPU/VOP3PInstructions.td:1908. LLVM explicitly uses ReadsModeReg = 1 for instructions such as MFMA whose floating-point behavior depends on MODE.
Consequently, rocjitsu currently makes identical WMMA instructions and operands produce different results depending on MODE, contrary to the public compiler contract.
Suggested fix: implement the instruction’s fixed rounding/denormal behavior and replace the MODE-varying tests with fixed-result vectors. If physical gfx1251 validation shows LLVM’s metadata is wrong, that compiler-contract discrepancy should be resolved explicitly before retaining MODE-dependent behavior.
Confidence: medium-high. The LLVM contract is explicit, but hardware validation is still needed to establish the exact fixed arithmetic rules.
Add the generated execution callback for V_WMMA_F64_16X16X4_F64 and route it through a CDNA5 wave32 helper. The public LLVM gfx1251 profile defines a 16-register F64 destination and accumulator tuple, four-register A and B tuples, modifier encodings, and the inline 1.0 accumulator form. Combine that contract with the checked-in CDNA5 WMMA data formats to map each logical F64 component to consecutive VGPR words. Stage all inputs before writing the destination so overlapping tuples retain read-before-write behavior, use the existing mode-aware fused F64 primitive for each reduction step, and apply EXEC to destination writes. Reject reserved layouts, unsupported modifiers and sources, out-of-range or misaligned register tuples, DPP, and wave64. Cross-check legal encodings, invalid operands, and tuple boundaries with public LLVM MC. Cover full matrix mapping, final tuple boundaries, overlap, partial EXEC, composed modifiers, fused rounding, denorm modes, exceptional values, and invalid encodings. Keep normal gfx1251 simulation disabled through execution_implemented=false. Target-specific setreg behavior and functional simulator enablement remain separate changes. The lane and register mapping and FP-mode behavior are source-derived and still require differential validation on physical gfx1251 hardware. Test: LLVM MC and FileCheck; amdisa tests; rocjitsu_tests; CTest.
Accept the complete public LLVM VISrc_512_f64 integer and FP64 inline-accumulator selector ranges and resolve their values at full 64-bit width. Make F64 WMMA arithmetic independent of wavefront MODE, matching LLVM ReadsModeReg=0. Use a fixed round-to-nearest-even, preserve-denorm baseline while retaining the physical-hardware validation TODO. Update decoder and execution coverage for the newly accepted inline forms and fixed arithmetic policy. Check Result-returning execute_instruction calls required by the rebased upstream API. Test: amdisa generator/sema/provenance tests; rocjitsu_tests build; focused gfx1251 packed-F64 and F64-WMMA CTest; pre-commit.
1aae284 to
5daf283
Compare
❌ PR Check — Action Required
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🚫 Please fix the failed policies before requesting reviews. The following policy checks failed:
The |
There was a problem hiding this comment.
🔵 Needs a closer look
A moderate generated-validation defect remains, DPP rejection coverage is incomplete, and the change requires human and hardware validation.
Pull request overview
Implements gfx1251 F64 WMMA execution through the CDNA5 wave32 path while keeping normal gfx1251 simulation disabled.
Changes:
- Adds generated execution and callback/backend wiring.
- Implements matrix mapping, validation, modifiers, fused arithmetic, and EXEC masking.
- Adds execution, semantic, and generator test coverage.
File summaries
| File | Reviewed changes |
|---|---|
shared/machine-readable-isa/isa/amdgpu_isa_cdna5_variants.json |
Updates F64 WMMA execution metadata. |
emulation/rocjitsu/tests/cdna5_execution_test.cpp |
Adds execution, mapping, arithmetic, masking, overlap, and validation tests; DPP16/DPP8 rejection coverage remains incomplete. |
emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/isa/arch/amdgpu/shared/mma_exec.h |
Adds the CDNA5 wave32 F64 WMMA helper and mappings. |
emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/isa/arch/amdgpu/generated/cdna5/vop3p.h |
Declares the generated instruction execution interface. |
emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/isa/arch/amdgpu/generated/cdna5/vop3p.cpp |
Adds decoding and operand validation. |
emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/isa/arch/amdgpu/generated/cdna5/vop3p_exec.cpp |
Routes execution to the F64 WMMA helper. |
emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/isa/arch/amdgpu/generated/cdna5/execution_backend.h |
Adds the execution identifier. |
emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/isa/arch/amdgpu/generated/cdna5/execution_backend_exec.cpp |
Registers the execution callback. |
emulation/rocjitsu/lib/python/amdisa/tests/test_sema_derive.py |
Tests F64 WMMA semantic derivation. |
emulation/rocjitsu/lib/python/amdisa/tests/test_generator_profile_gates.py |
Tests generator execution and validation gates. |
emulation/rocjitsu/lib/python/amdisa/semantics.py |
Recognizes F64 WMMA signatures. |
emulation/rocjitsu/lib/python/amdisa/codegen/execute/matrix.py |
Generates the specialized matrix execution path. |
emulation/rocjitsu/lib/python/amdisa/codegen/_generator.py |
Generates validation and callback wiring; src0 validation lacks the required lower-bound check for invalid selectors. |
Review details
Suppressed comments (2)
emulation/rocjitsu/lib/python/amdisa/codegen/_generator.py:10430
src0is constructed asOPR_SRC_VGPR, whose legal selectors start at 256, but this generated check only rejects values above 508. As a result, the addedsrc0 = 0/242cases are accepted instead of rejected, and execution can resolve an invalid VGPR offset; include the lower bound here and regeneratevop3p.cpp.
f'if ({raw_inst}->{src_name} > 508u) '
emulation/rocjitsu/tests/cdna5_execution_test.cpp:2276
- The new rejection matrix does not exercise either DPP16 (
SRC_DPP) or DPP8 (SRC_DPP8_FI_0/1), even though this instruction has a custom DPP rejection path and its tuple checks run before that path. Add decode-failure cases using the actual DPP extension encodings so a regression that accepts or mis-parses a DPP form is caught.
TEST(Gfx1251F64WmmaExecutionTest, RejectsUnsupportedControlsSourcesAndRegisterTuples) {
auto decoder =
make_isa_decoder<cdna5::Isa>(&cdna5::execution_backend(), cdna5::kGfx1251IsaFeatures);
ASSERT_NE(decoder, nullptr);
constexpr cdna5::Vop3pBuilderFields kValid{
.vdst = 8, .src0 = 256, .src1 = 260, .src2 = 264, .opsel_hi = 3};
const auto rejected = [&](cdna5::Vop3pBuilderFields fields) {
const auto words = cdna5::build_vop3p(cdna5::kVWmmaF6416x16x4F64Vop3p, fields);
EXPECT_EQ(decode_valid(*decoder, words.data()), nullptr);
- Files reviewed: 8/13 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
kuhar
left a comment
kuhar
left a comment
There was a problem hiding this comment.
Please update the PR description to match this revision. It still says the implementation is mode-aware and covers denormal modes, but the helper now uses a fixed RNE/preserve-denorm policy and tests MODE independence. The body also says partial EXEC is covered as supported behavior, while CDNA5 ISA section 7.12 requires EXEC to be all ones. Keeping those statements aligned with the code and remaining hardware-validation caveat will prevent reviewers from assessing the superseded contract.
Signed-off-by: hanhanW <hanhan0912@gmail.com>
TheRock Submodule Bump ActivityNewest first
|
The public LLVM gfx1251 profile exposes
V_WMMA_F64_16X16X4_F64, butrocJITsu previously treated the instruction as model-only and did not emit an
execution callback. Add semantic derivation, generated decode validation and
callback wiring, and a CDNA5 wave32 execution helper so compiler-produced forms
can execute in functional simulation.
Enforce the public operand contract: a 16-VGPR destination/accumulator tuple,
4-VGPR A and B tuples, supported source modifiers and matrix-reuse hints, and
the standard integer and FP64 inline accumulator selectors. Reject reserved
control bits, invalid or misaligned register tuples, and DPP/DPP8 extension
encodings.
CDNA5 requires
EXECto contain all 32 wave lanes for WMMA instructions.Reject wave64 or non-full-
EXECexecution before resolving or reading operands.Report this through rocJITsu's instruction-execution failure path instead of
using exceptions for control flow.
Stage every source before writing the destination so legal overlapping tuples
preserve read-before-write behavior. Evaluate each reduction with a fixed
round-to-nearest-even, preserve-input/output-denormals policy independent of the
wavefront
MODEregister, consistent with LLVM marking the instruction withReadsModeReg = 0.The logical operation and encoding forms come from public LLVM support. The
lane/register mapping combines the public CDNA5 ISA description with the
checked-in WMMA format metadata. The exact fixed arithmetic policy and physical
lane/register behavior still require validation on gfx1251 hardware; this
change does not claim that source-derived behavior is hardware-confirmed.