Repository navigation
[rocjitsu] Model RDNA integer WMMA and SWMMAC - #13811
Conversation
✅ All Checks Passed — Ready for Review
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
Pre-commit check failed⛔ pre-commit failed Please run locally:
This repo uses |
kuhar
left a comment
kuhar
left a comment
There was a problem hiding this comment.
We should retarget the existing RDNA4 sparse-layout oracle to the new production helper. In emulation/rocjitsu/tests/shared_infra_test.cpp:834, SwmmacI32K64U4VaryingSelectorsMatchSparseReference still calls exec_swmmac_i32, but generated RDNA4 code now calls exec_rdna_swmmac_i32<4, 64>. The test can pass even if the new index-set reader or accumulation-order mapping is wrong.
The review's scratch version passes with exec_rdna_swmmac_i32<4, 64>(*cu, 32, d_base, a_base, b_base, index_base, 0, false, false, false); D is already initialized to zero. Please also route the RDNA4 K32 IU8 case in emulation/rocjitsu/tests/simd_correctness/wmma_simd_exact_test.cpp:603 through the production path so its scalar/SIMD comparison covers the implementation actually dispatched. Generic-helper coverage for other architectures can remain.
We should retain decoded regressions for the independent behavior introduced here. The current tests pass, but these cases are missing:
- GFX11 IU4 four-product saturation (
emulation/rocjitsu/tests/rdna_wmma_i32_test.cpp:18). A scratch change from group size 4 to final-only saturation still passed both new tests. A small witness uses signed IU4, CLAMP, C=0x7ffffff5, A rows[7,7,7,7,-8,-8,-8,-8,0,0,0,0,0,0,0,0], and B filled with 7: the result is0x7fffff1f, versus0x7fffffd9with final-only saturation. The additional GFX11 hardware capture agrees with the current implementation; the missing piece is a retained regression. - Sparse IU4 K32 key 1 (
emulation/rocjitsu/tests/rdna_swmmac_i32_test.cpp:10) selects a 16-bit set that neither current sparse case exercises. Please retain that witness and run the sparse cases with bothforce_scalarsettings, as the dense test already does. - Inline C=
-1(emulation/rocjitsu/lib/python/amdisa/codegen/execute/matrix.py:507). The new optional accumulator fixes a collision withACC_FROM_VGPR == 0xffffffff; previously it read stale D instead. A decoded case with A=B=0 and D prefilled with0x55should produce0xffffffff. All checked-in new dense cases currently use a VGPR accumulator.
These target separate policy, index-selection, and operand-source branches without requiring an exhaustive shape matrix.
I think we should keep the captured data independent of the execution harness and list it in the fixture index.
emulation/rocjitsu/tests/fixtures/float_dot/rdna_integer_samples.h:7 includes rdna_matrix_test_util.h solely for Sample, bringing the decoder, compute unit, and GoogleTest into a data header. Defining the small sample record alongside the captures (or in a lightweight shared data header) would let the fixture be reused without the simulator harness, as neighboring fixture headers already allow.
Please also add rdna_integer_samples.h to the contents table in emulation/rocjitsu/tests/fixtures/float_dot/README.md:8. The new prose mentions rdna_*_test.cpp but never identifies the header containing the captures. No directory move is needed.
09cb494 to
323785f
Compare
|
Addressed the review: retargeted the sparse-layout and K32 IU8 tests to the production helpers; added compact witnesses for GFX11 four-product saturation, sparse IU4 K32 key 1 (both scalar settings), and inline C=-1. The sample record is now independent of the simulator harness, and the fixture is indexed. Validation at this commit: 4 focused C++ tests, 123 matrix generator tests, and all applicable pre-commit hooks pass. The earlier emulation CI failure is separate: all 263 reported test failures also appear in the base develop job. This PR's new integer tests passed there; the job subsequently ended with a custom-container error. The push starts a fresh CI run. |
|
🎉 All checks passed! This PR is ready for review. |
Saturate at intermediate product-group boundaries and use the RDNA-specific sparse index slices and accumulation order. Preserve overlapping operands by staging destination writes. Keep a few decoded regressions from gfx1100/gfx1201 captures for saturation, wave layouts, and sparse index overlap. Co-authored-by: Benoit Jacob <jacob.benoit.1@gmail.com>
323785f to
aec913d
Compare
TheRock Submodule Bump ActivityNewest first
|
Originally by @kuhar, with cleanup by @bjacob .
Saturate integer WMMA accumulators at intermediate product-group boundaries and use RDNA-specific accumulation order and sparse index selection.
Stage destination writes so overlapping sources and sparse indices remain readable throughout the instruction.
Add a small set of decoded regressions for saturation, wave layouts, and operand overlap.
Validation: GCC Release build and 22 focused C++ tests pass at the four-commit stack tip; generator-profile tests also pass.
Progress towards #13670.