[CloudP4bbs]

One line removed from a guard in the Ampere to Maxwell lowering.

What was rejected

In the IADD3 with carry-out path, the uniform forms were refused when the carry-out predicate index was above 1:

if ((carry_in && carry_out != 7) || (opcode == 0x210 && (insn.words[1] & ~0xffU)) ||
    (rewritten_uniform && carry_out != 7 && carry_out > 1) ||          // <- this clause
    (rewritten_uniform && carry_in && insn.Bits(87, 3) > 1 && insn.Bits(87, 3) != 7))
    return fail(index, "native integer carry mode");

The third clause is the one that fires. The lowering directly below it already handles the uniform case: it picks the target with 6 - carry_out, emits the compare that produces the carry, writes it into the uniform bank (R239, bit 6-n) and restores the predicate around it. So the clause was refusing a shape the code below can already lower.

Why it matters for Ocarina of Time

That title emits [op 290 0f067290 00000006 0ff5e03f 000fe200], where Bits(81, 3) == 3: uniform IADD3 with a carry-out, no carry-in. It lands exactly in the removed clause, and the program is discarded with reason native integer carry mode.

Measured before and after

Same machine, RX 5700, Windows 11, amdvlk 32.0.21045.16010, clean cache/shader/0400486024dec000 before each run, EDEN_NX2_SPLIT=2 and EDEN_NX2_FENCE_BATCH=1, 0400486024DEC000. Both runs also carry the 0x4a4 lowering from #604, which is the other rejection this title produced; without #604 this one is not reached.

before this change   1 rejected program, reason "native integer carry mode"
after this change    0 rejected programs, 4800 to 7800 presented frames in 260 to 280 seconds

The two runs differ in duration because of pipeline compilation on a cold cache, so the meaningful number is the rejection count, not the frame count.

What I did not change

carry_in combined with a uniform carry_out is still refused, and carry_out for the non-uniform forms is untouched. I only tested the case above, with the title that produces it, so this is intentionally the smallest change that makes that program lower.

If a reviewer would rather relax it differently, for example keeping the clause for carry_out > 6 as a defensive check, that also works for this case and I can adjust.

Summary by CodeRabbit

• Bug Fixes
  • Fixed an issue where certain carry outputs from IADD3 operations were rejected when a carry output was requested. These outputs can now proceed through the existing uniform-predicate mapping.

[coderabbitai]

No actionable comments were generated in the recent review.

Walkthrough

The IADD3 carry-mode validation no longer rejects rewritten-uniform carry outputs 2–6 when a carry output is requested. These values proceed to the existing predicate-slot mapping.

Changes: IADD3 carry outputs

Layer / File(s)	Summary
Allow rewritten-uniform carry outputs src/shader_recompiler/frontend/ampere/lower_to_maxwell.h	The validation that rejected rewritten-uniform carry outputs above 1 was removed. Outputs 2–6 can proceed to the existing predicate-slot mapping.

Estimated code review effort: 1 (Trivial) | ~3 minutes

Change: Bug fix

Possibly related PRs

• Djihads80/Eglantine#604: Adds UIMAD to the uniform-opcode rewrite list and addresses an earlier step in the same program-rejection path.

Suggested reviewers: camillelavey

Merge Risk: Minimal · up to 25e1b

The change allows additional uniform carry outputs without an identified merge-blocking issue.

Architecture risk: Low · up to 25e1b

The change affects 1 system.

Changed systems: src

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

• observed — src (ui) was modified; 1 changed file maps to changed impact.

Before / after behavior

• observed — Modified behavior in src/shader_recompiler/frontend/ampere/lower_to_maxwell.h: Removed the validation that rejected rewritten-uniform carry outputs above 1 unless the output was disabled (7). Rewritten-uniform outputs 2–6 now proceed to the existing predicate-slot mapping.

Pre-merge checks: 5

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely describes the main change: allowing uniform IADD3 carry-out values above 1 during shader lowering.
Docstring Coverage		No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Full details: Docstring Coverage

Explanation

No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 0 files.

Finishing Touches

Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Warning

Some tools did not complete. Review the errors below.

ast-grep (0.51.1)

src/shader_recompiler/frontend/ampere/lower_to_maxwell.h

ast-grep timed out on this file

Recent review info

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Repository UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: dbb37e97-8028-4e6c-a12e-475104305cef

Commits

Reviewing files that changed from the base of the PR and between cb60749 and 25e1bdf.

Files selected for processing: 1

• src/shader_recompiler/frontend/ampere/lower_to_maxwell.h

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

[CloudP4bbs]

Confirmation with the clean patch, rebuilt from this branch alone on top of nx2 plus #604:

build c5dbdabb   rejected programs 0   presented frames 8400 in 260 s   ran to the end of the run

That is the same result as the first measurement, now from a source tree that contains only this change and #604, so the frame count difference against the earlier 4800 was indeed cold pipeline compilation and not something this patch introduced.

For the record, the rejections this title produced before the two patches together were 0x4a4 (uniforms, #604) and then native integer carry mode (#638). With both applied, the boot of 0400486024DEC000 is clean on this driver, which is what I needed for the next step here, since a discarded program makes everything I capture afterwards unreliable.

[CloudP4bbs]

Closing this. It causes a regression that I found on my own machine, and it is worse than the rejection it removes.

What the change did

Removing the (rewritten_uniform && carry_out != 7 && carry_out > 1) clause let the OOT program lower instead of being discarded, and the rejection count at boot went from 1 to 0. That part was real.

What I did not check before opening it

I did not look at the rendered output. On this machine, with that clause removed, the title boots with zero rejections and presents 9600 frames in 300 s, and the 3D scene is completely black: the intro subtitle draws at 32 FPS over an empty background. The frame dump shows pass000 empty before any upscale touches it.

With the clause restored, same tree, same machine, same title:

guard removed (this PR)    rejected programs 0    presented 9600    screenshot 20 KB, black scene
guard restored (nx2 tree)  rejected programs 1    presented 7200    screenshot 2.4 MB, scene renders

The 20 KB against 2.4 MB is the same client area captured with PrintWindow from the emulator's own window handle, so this is the scene and not a desktop screenshot. At the restored build the image shows the intro tapestry with the Deku Tree, Hyrule Castle and the forest, subtitle "At the very edge of this kingdom, there lay a small forest", 30 FPS.

Why the removal is wrong

The clause was not conservative, it was a guard on a shape the lowering below does not actually handle: carry_out above 1 in the uniform form does not map to the single 6 - carry_out target the code emits. Lowering those programs with that assumption produces shaders that build, run and draw nothing, which is why there is no rejection, no error and no crash to point at.

So the correct statement for this title is: the program is rejected for a reason, and making the rejection go away without implementing the carry-out properly moves the failure from "program discarded" to "geometry silently missing". The rejection was the honest behaviour.

What is left

The 0x4a4 lowering in #604 is separate and still stands on its own. The carry-out case needs the real implementation, which for carry_out in 2 to 6 means writing more than one predicate output, not picking one with 6 - carry_out. I have not written that yet and will not guess at it.

Thanks to anyone who looked at this one; sorry for the noise.