fix(shaders): LDG/STG [URn+imm] with ordering bit 78 lower (2 compute programs) #1778
No reviewers
Labels
No labels
accessibility
autorelease: pending
autorelease: tagged
bug
dependencies
documentation
duplicate
enhancement
github_actions
good first issue
help wanted
invalid
question
testing wanted
wontfix
No milestone
No assignees
1 participant
Due date
No due date set.
Dependencies
No dependencies set
Reference
MagicBeanDev/Hookshot!1778
Loading…
Reference in a new issue
No description provided.
Delete branch "fix/lower-ldg-uniform-order"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Two of OoT's compute programs fail to translate ("does not lower ... native global load uniform addressing mode"), so their dispatches do nothing on every PC. Both load from a uniform-register address with memory-ordering bit 78 set beside bit 77, and the lowering only allowed 77.
The encodings (startup pipeline build, Linux; also in a Radeon 890M v2.0.13 log from #bug-reporting):
These are the uniform-address forms the lowering already handles (
0x081e8100for LDG,0x0810a100for STG, bit 79 set, bit 76 clear). The only extra is bit 78. Bit 77, in the same ordering field, is already ignored ("needs nothing extra from a Maxwell LDG"), because Maxwell's LDG/STG have no ordering qualifier. The second program got past its load and then stopped at the store, so the store is included here.Change (
lower_to_maxwell.h): bit 78 is allowed beside 77 in the uniform-address LDG (0x981) and STG (0x986). They lower to Maxwell LDG.E / STG.E of the uniform register pair plus the offset, as before. Bit 76 and other unknown bits in the uniform form stay refused, as do odd uniform registers.Tests
tests/test_lower_ldg_uniform_order.py(inrun_all.py): the three encodings lower to LDG.E / STG.E with the right destination or data, offset, size,.Eand an aligned address pair; bit 77 alone or neither lower as before; bit 76 is refused. It fails onmainand passes here.run_all.py: 246 passed, 0 failed.Ordering note: lowering a strong load or store to a plain one could matter only across workgroups, for example a spin on a flag another workgroup writes. I haven't disassembled these two programs to rule that out. Reviewers who know these passes may want to check. The register form has accepted bit 77 the same way.
🤖 Generated with Claude Code
Thanks, the mask change itself is right:
0x0c1ee900/0x0c1eed00reduce to0x081e8100under the new LDG mask,0x0c10e906to0x0810a100under the STG one, bit 76 and odd URs are still refused, and the test builds the real header and is inrun_all.py.My one real concern is the ordering note you already raised. In this same file the
.STRONG.GPUatomics carry this field as bits 77-79 all set (RED0x98e0x0c10e100,ATOMG0x3a80x001ee100, 0x9a80x081ee140). So these three are most likelyLDG.E.STRONG.GPU/STG.E.STRONG.GPU, not plainLDG.E. That's what volatile or relaxed-atomic loads and stores compile to, which is the cross-workgroup case. On our side nothing carries that through. The SPIR-V back end puts noCoherent/Volatileon storage buffers, so once the qualifier is gone the driver may hoist a load out of a spin loop (GPU hang instead of today's no-op dispatch) or serve it from L1.There's already a cheap precedent in the file:
SUST ... .STRONG.GPU(case 0xb9b) is bracketed withMEMBAR.GLon both sides, which ends up as anOpMemoryBarrierat Device scope and stops the compiler moving or merging the access across it. Could you do the same here when bit 78 is set (or 77+78)? For two programs the cost is nothing, and then we don't have to disassemble them to be sure. Inline note on the LDG line.Nits:
0xc), which isn't an encoding we've seen or tested. Either require 77 when 78 is set or add a case for it. STG already requires 77 through its base pattern, so it's fine.LDG.E/STG.Ewith no ordering suffix. Worth labelling them.STRONG.GPUso the next reader doesn't take them for weak accesses.CI hasn't run yet (all checks pending).
These look like
.STRONG.GPU(bits 77-79 set, same as the RED/ATOMG cases above). Suggest mirroring the SUST.STRONG.GPUhandling at 0xb9b: wheninsn.Bits(78, 1)is set, emitMEMBAR.GL(0xef98000000000000ULL | predicate | (u64{1} << 8)) before and after theLDG.E, and the same around theSTG.Ebelow. Without that, a strong load ends up as a plain SPIR-VOpLoadon a non-Coherentbuffer, which the driver may hoist out of a polling loop.Nit: this mask also lets 78 through without 77.
Heads-up: this conflicts with main now (a lot landed today). Could you merge main into the branch (or rebase) and resolve the conflicts? Thanks!
Thanks, agreed. Bits 77-79 all set read as
.STRONG.GPUnext to the RED/ATOMG encodings. Done in the new commit: with bit 78, the LDG.E and the STG.E are each fenced byMEMBAR.GL(0xef98000000000000 | predicate | 1 << 8) before and after, mirroring SUST.STRONG.GPU at 0xb9b. The test checks two fences around each of the log's strong accesses, and none for bit 77 alone or neither. I also removed the ordering caveat from the description. Rebased onto main's newrun_all.py(RUN_ALL_ARGSin the test);run_all.py: 263 passed, 0 failed.Correction to my last comment: I didn't rebase this one. I saw your merge of main into the branch, so I kept it, merged the current main on top (the new
run_all.py, withRUN_ALL_ARGSin the test), and added the MEMBAR commit. Plain push, no force.Thanks for the quick turnaround. The ordering point is fixed: with bit 78 set, the uniform LDG.E (0x981) and STG.E (0x986) are now each fenced by MEMBAR.GL before and after, with the same encoding SUST.STRONG.GPU uses at 0xb9b. Both fences use the instruction's predicate. The descriptor forms can't pick it up, because their masks don't allow bit 78. The test now checks two fences around each of the three logged encodings and none for bit 77 alone. The merge of main resolves run_all.py cleanly, and the branch is mergeable.
Nits, none blocking:
(hookshot)comments on the masks (around lines 1910 and 1958) still say bit 78 needs "nothing for a Maxwell LDG/STG", which no longer matches the fenced lowering.Heads-up on overlap: #1830 (super build 1/4) makes the same two mask changes (0x6000 for LDG, 0x4000 for STG) but without the fences. Whichever lands second will conflict in those hunks. If #1830 goes first, please keep the MEMBAR.GL fences when you resolve it.
CI was still running when I looked (only the F2I check had finished, and it passed).
@ -1891,2 +1891,4 @@const u32 size = insn.Bits(73, 3);u32 address = insn.Bits(24, 8);// (hookshot) bits 77-79 all set: .STRONG.GPU, as the atomics in this file carry it (RED, ATOMG). A Maxwell LDG has// no ordering: fenced on both sides with MEMBAR.GL, as SUST.STRONG.GPU (0xb9b) is (hckrman101, PR 1778)Nit: could you drop
(hckrman101, PR 1778)here? The other(hookshot)notes in this file don't name reviewers, and the SUST reference already explains where this comes from.