[feikou]

Motivation

ds_read_b64_tr_b16 returns incorrect results under the gfx950 (CDNA4) simulator. The instruction performs a 4x16-lane halfword transpose: within each 16-lane group, destination lane l halfword n comes from source lane ((l & ~0xf) | ((l >> 2) & 3)) + 4 * n, halfword l & 3 — the layout already encoded by the DBT reference in tests/dbt/cdna4_to_cdna3_lds_hip_test.cpp (LargeStaticLdsReadB64TrB16). The pure-simulation path's transpose_b16() instead performed a 4x4 transpose within groups of 4 consecutive lanes (lane 0 received source lanes 0,1,2,3 halfword 0 instead of lanes 0,4,8,12), so every gfx950 kernel that loads bf16/f16 MFMA operands with ds_read_b64_tr_b16 computed on wrong data under the simulator.

Technical Details

• lib/rocjitsu/src/rocjitsu/isa/arch/amdgpu/shared/ds_transpose.h: transpose_b16() now handles the wave64 B64 case (4 halfwords per lane, i.e. ds_read_b64_tr_b16) with the 4x16-lane stride-4 layout above. This changes only that path; the B128 path used by gfx1250 ds_load_tr16_b128 / global_load_tr16_b128 and RDNA4 global_load_tr_b128 is left byte-for-byte unchanged. tr_b4 / tr_b6 / tr_b8 are untouched and were not hardware-verified here.
• tests/amdgpu_vm_test.cpp: new DsTransposeTest.ReadB64TrB16_LaneLayout (CPU-only). Fills LDS with a per-lane halfword pattern, dispatches an encoded ds_read_b64_tr_b16 on the cdna4 VM fixture, and checks all 64 lanes x 4 halfwords against the same reference formula the DBT test uses.
• TransposeKind is split into TR16_B128 (gfx1250/RDNA4 B128 loads, unchanged) and B64_TR_B16 (CDNA4 ds_read_b64_tr_b16), emitted from InstructionSemantics; transpose_response() dispatches on the kind.

Issue Tracking

No existing GitHub issue. Related: #6829 (AccVGPR acc bit handling for the same DS transpose reads).

Test Plan

• ctest / rocjitsu_tests --gtest_filter='DsTransposeTest.*' for the new CPU-side layout test, before and after the transpose_b16() change.
• Existing test suite for regressions.

Test Result

• DsTransposeTest.ReadB64TrB16_LaneLayout: built and run locally (CPU only, GCC 13, Release): fails on the pre-change transpose_b16() with 128 mismatches (64 lanes x 2 dwords; e.g. lane 0 low dword 0x12111200 vs expected 0x12441200) and passes after.
• Cross-checked on gfx950 hardware (ROCm 7.2.0) with an equivalent inline-asm ds_write_b64/ds_read_b64_tr_b16 kernel: hardware matches the expected layout; the simulator matched only after this change (all 64 lanes differed before).
• Full ctest shows no status changes versus develop other than the added DsTransposeTest.ReadB64TrB16_LaneLayout.

Submission Checklist

• Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md.

[therock-pr-bot]

✅ All Policy Checks Passed

Check	Status	Details
📝 PR Description	✅ Pass	—
⛔ Forbidden Files	✅ Pass	—
🧪 Unit Test	✅ Pass	—
🚫 Draft PR	🔜 To Be Enabled	—
🚩 Feature Flag	🔜 To Be Enabled	—
📊 Code Coverage	🔜 To Be Enabled	—

🎉 All policy checks passed!

📖 Need help? See the Policy FAQ for details on every check and how to fix failures.

🙋 Wish to Override Policy?

[therock-pr-bot]

🎉 All checks passed! This PR is ready for review.

[lua1235]

Thanks for fixing this! The changes look good, just 2 small comments.

[lua1235]

Please also register this test with ctest. rj_add_hip_test only creates the build target.

eg: rj_add_hip_test_case(hip_ds_read_tr_b16_sim_test "DsReadTrB16SimTest.LaneLayout")

[lua1235]

Could we preserve this layout distinction in InstructionSemantics instead of inferring it here from read width and wave size?

TransposeKind::TR_B16 is now both CDNA4 ds_read_b64_tr_b16 and B128 TR16 loads. Since these are distinct layouts, please split them into separate transpose/layout kinds and dispatch on that here.

[feikou]

SG, let me refactor this part. thanks for the reivew!

[feikou]

Done in a466e5a — split into TR16_B128 (id 4, gfx1250/RDNA4 B128 loads, unchanged) and a new B64_TR_B16 (id 5, CDNA4 ds_read_b64_tr_b16), emitted from InstructionSemantics; transpose_response() now dispatches on the kind. Gave the new id to the CDNA4 form so only cdna4/ds_exec.cpp needed regenerating.

[lua1235]

Looks good!

[kuhar]

LGTM, thanks for fixing this

[kuhar]

@feikou FYI, I root caused the tsan failure and it's novel but pre-existing and unrelated to your PR. I will wait for the test of the checks to finish before merging, and then fix up tsan in a follow up PR.

[feikou]

@feikou FYI, I root caused the tsan failure and it's novel but pre-existing and unrelated to your PR. I will wait for the test of the checks to finish before merging, and then fix up tsan in a follow up PR.

SG, thanks @kuhar for investigating and fixing the tsan!
I can rebase on top of yours later.

[kuhar]

Update @feikou: I have a fix up for review that should makes your test work properly under sanitizers: #10195 , but I think we want to drop the test file anyway for the same reason @atgutier stated in #10192 (review).
We prefer not to rely on HIP C++ tests w/ inline assembly and instead test in-process like in

rocm-systems/emulation/rocjitsu/tests/amdgpu_vm_test.cpp

Lines 3075 to 3126 in fd4771e

	// Verify that ds_read_b64_tr_b16 with acc=1 writes to AccVGPR (vb+256+vdst),
	// not to VGPR (vb+vdst).
	TEST(DsTransposeTest, ReadB64TrB16_AccBit) {
	VmFixture f("cdna4", 1, 10);
	auto *snap = f.capture_halts();
	constexpr uint32_t VDST = 4;
	constexpr uint32_t ADDR_REG = 0;
	constexpr uint32_t DS_OP = 227; // ds_read_b64_tr_b16
	// Kernel:
	// v_mov_b32 v0, 0 ; addr = LDS offset 0
	// v_mov_b32 v4, 0x42 ; sentinel in VGPR v4
	// v_mov_b32 v5, 0x42 ; sentinel in VGPR v5
	// ds_read_b64_tr_b16 a[4:5], v0 ; acc=1: write to AccVGPR
	// s_waitcnt lgkmcnt(0)
	// s_endpgm
	using namespace enc;
	const uint32_t code[] = {
	v_mov_b32(ADDR_REG, INLINE_CONST(0)),
	v_mov_b32(VDST, INLINE_CONST(42)),
	v_mov_b32(VDST + 1, INLINE_CONST(42)),
	ds_lo(DS_OP, /*offset0=*/0, /*offset1=*/0, /*acc=*/1),
	ds_hi(VDST, /*data0=*/0, ADDR_REG),
	S_WAITCNT_0,
	S_ENDPGM,
	};
	uint64_t ko = f.write_kernel(0x1000, code, sizeof(code));
	auto *cu = f.cu();
	// Write a known non-zero pattern to LDS.
	for (uint32_t i = 0; i < 256; ++i)
	cu->lds().write32(i * 4, 0xDEADBEEF);
	test::AqlQueue queue(f.mem(), f.cp());
	queue.dispatch(ko, 64);
	f.engine->run();
	ASSERT_EQ(snap->snapshots().size(), 1u);
	const auto &wf = snap->snapshots().front();
	// VGPR v4 should still hold the sentinel (42), not overwritten by ds_read.
	uint32_t vgpr_val = wf.vgpr(VDST, 0);
	EXPECT_EQ(vgpr_val, 42u) << "VGPR v" << VDST << " should NOT have been written when acc=1";
	// AccVGPR a4 should have been written with LDS data (not 42, not 0).
	uint32_t acc_val = wf.vgpr(256 + VDST, 0);
	EXPECT_NE(acc_val, 0u) << "AccVGPR a" << VDST << " should have been written by ds_read";
	EXPECT_NE(acc_val, 42u) << "AccVGPR a" << VDST
	<< " should contain LDS data, not the VGPR sentinel";
	}

.

[feikou]

Done in 1314f99 — dropped the HIP test file and its CMake registration; the in-process DsTransposeTest.ReadB64TrB16_LaneLayout stays. Thanks for #10195.