fix(shaders): LDG/STG [URn+imm] with ordering bit 78 lower (2 compute programs) #1778

Merged
Joey-Magnus merged 5 commits from fix/lower-ldg-uniform-order into main 2026-11-10 18:06:22 +00:00
Joey-Magnus commented 2026-11-10 16:27:04 +00:00 (Migrated from github.com)

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):

a7a54ff1af0cfa19 +0x10   LDG  ff017981 00002c04 0c1ee900 000ea800   LDG.E R1, [UR4+0x2c], 32 bits
a09bdc185d560567 +0x80   LDG  ff087981 00002004 0c1eed00 000ea200   LDG.E R8, [UR4+0x20], 128 bits
a09bdc185d560567 +0x720  STG  ff000986 00003007 0c10e906 000fee00   STG.E [UR6+0x30], R7, 32 bits

These are the uniform-address forms the lowering already handles (0x081e8100 for LDG, 0x0810a100 for 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

  • New tests/test_lower_ldg_uniform_order.py (in run_all.py): the three encodings lower to LDG.E / STG.E with the right destination or data, offset, size, .E and an aligned address pair; bit 77 alone or neither lower as before; bit 76 is refused. It fails on main and passes here.
  • run_all.py: 246 passed, 0 failed.
  • In game (Linux, RTX 4070 Ti, 595.99.02), stacked on #1769: programs that do not lower go 7 → 5 (both gone), and the build's failed-program count 19 → 17.

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

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): ``` a7a54ff1af0cfa19 +0x10 LDG ff017981 00002c04 0c1ee900 000ea800 LDG.E R1, [UR4+0x2c], 32 bits a09bdc185d560567 +0x80 LDG ff087981 00002004 0c1eed00 000ea200 LDG.E R8, [UR4+0x20], 128 bits a09bdc185d560567 +0x720 STG ff000986 00003007 0c10e906 000fee00 STG.E [UR6+0x30], R7, 32 bits ``` These are the uniform-address forms the lowering already handles (`0x081e8100` for LDG, `0x0810a100` for 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** - New `tests/test_lower_ldg_uniform_order.py` (in `run_all.py`): the three encodings lower to LDG.E / STG.E with the right destination or data, offset, size, `.E` and an aligned address pair; bit 77 alone or neither lower as before; bit 76 is refused. It fails on `main` and passes here. - `run_all.py`: 246 passed, 0 failed. - In game (Linux, RTX 4070 Ti, 595.99.02), stacked on #1769: programs that do not lower go 7 → 5 (both gone), and the build's failed-program count 19 → 17. 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](https://claude.com/claude-code)
hckrman101 (Migrated from github.com) reviewed 2026-11-10 16:32:27 +00:00
hckrman101 (Migrated from github.com) left a comment

Thanks, the mask change itself is right: 0x0c1ee900 / 0x0c1eed00 reduce to 0x081e8100 under the new LDG mask, 0x0c10e906 to 0x0810a100 under the STG one, bit 76 and odd URs are still refused, and the test builds the real header and is in run_all.py.

My one real concern is the ordering note you already raised. In this same file the .STRONG.GPU atomics carry this field as bits 77-79 all set (RED 0x98e 0x0c10e100, ATOMG 0x3a8 0x001ee100, 0x9a8 0x081ee140). So these three are most likely LDG.E.STRONG.GPU / STG.E.STRONG.GPU, not plain LDG.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 no Coherent/Volatile on 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 with MEMBAR.GL on both sides, which ends up as an OpMemoryBarrier at 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:

  • The LDG mask accepts bit 78 without 77 too (field 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.
  • The disassembly in the description prints LDG.E / STG.E with no ordering suffix. Worth labelling them .STRONG.GPU so the next reader doesn't take them for weak accesses.

CI hasn't run yet (all checks pending).

Thanks, the mask change itself is right: `0x0c1ee900` / `0x0c1eed00` reduce to `0x081e8100` under the new LDG mask, `0x0c10e906` to `0x0810a100` under the STG one, bit 76 and odd URs are still refused, and the test builds the real header and is in `run_all.py`. My one real concern is the ordering note you already raised. In this same file the `.STRONG.GPU` atomics carry this field as bits 77-79 all set (`RED` 0x98e `0x0c10e100`, `ATOMG` 0x3a8 `0x001ee100`, 0x9a8 `0x081ee140`). So these three are most likely `LDG.E.STRONG.GPU` / `STG.E.STRONG.GPU`, not plain `LDG.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 no `Coherent`/`Volatile` on 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 with `MEMBAR.GL` on both sides, which ends up as an `OpMemoryBarrier` at 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: - The LDG mask accepts bit 78 *without* 77 too (field `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. - The disassembly in the description prints `LDG.E` / `STG.E` with no ordering suffix. Worth labelling them `.STRONG.GPU` so the next reader doesn't take them for weak accesses. CI hasn't run yet (all checks pending).
hckrman101 (Migrated from github.com) commented 2026-11-10 16:32:27 +00:00

These look like .STRONG.GPU (bits 77-79 set, same as the RED/ATOMG cases above). Suggest mirroring the SUST .STRONG.GPU handling at 0xb9b: when insn.Bits(78, 1) is set, emit MEMBAR.GL (0xef98000000000000ULL | predicate | (u64{1} << 8)) before and after the LDG.E, and the same around the STG.E below. Without that, a strong load ends up as a plain SPIR-V OpLoad on a non-Coherent buffer, which the driver may hoist out of a polling loop.

Nit: this mask also lets 78 through without 77.

These look like `.STRONG.GPU` (bits 77-79 set, same as the RED/ATOMG cases above). Suggest mirroring the SUST `.STRONG.GPU` handling at 0xb9b: when `insn.Bits(78, 1)` is set, emit `MEMBAR.GL` (`0xef98000000000000ULL | predicate | (u64{1} << 8)`) before and after the `LDG.E`, and the same around the `STG.E` below. Without that, a strong load ends up as a plain SPIR-V `OpLoad` on a non-`Coherent` buffer, which the driver may hoist out of a polling loop. Nit: this mask also lets 78 through without 77.
hckrman101 commented 2026-11-10 17:50:37 +00:00 (Migrated from github.com)

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!

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!
Joey-Magnus commented 2026-11-10 17:58:32 +00:00 (Migrated from github.com)

Thanks, agreed. Bits 77-79 all set read as .STRONG.GPU next to the RED/ATOMG encodings. Done in the new commit: with bit 78, the LDG.E and the STG.E are each fenced by MEMBAR.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 new run_all.py (RUN_ALL_ARGS in the test); run_all.py: 263 passed, 0 failed.

Thanks, agreed. Bits 77-79 all set read as `.STRONG.GPU` next to the RED/ATOMG encodings. Done in the new commit: with bit 78, the LDG.E and the STG.E are each fenced by `MEMBAR.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 new `run_all.py` (`RUN_ALL_ARGS` in the test); `run_all.py`: 263 passed, 0 failed.
Joey-Magnus commented 2026-11-10 17:59:40 +00:00 (Migrated from github.com)

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, with RUN_ALL_ARGS in the test), and added the MEMBAR commit. Plain push, no force.

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`, with `RUN_ALL_ARGS` in the test), and added the MEMBAR commit. Plain push, no force.
hckrman101 (Migrated from github.com) approved these changes 2026-11-10 18:05:30 +00:00
hckrman101 (Migrated from github.com) left a comment

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:

  • The two older (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.
  • The load mask still accepts bit 78 without 77. That's harmless now, since it just gets fenced.

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).

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: - The two older `(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. - The load mask still accepts bit 78 without 77. That's harmless now, since it just gets fenced. 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)
hckrman101 (Migrated from github.com) commented 2026-11-10 18:05:30 +00:00

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.

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.
Commenting is not possible because the repository is archived.
No description provided.