[joshuaswarren]

[Joshua's agent]

What

The rules that collapse x2xN(y2yM(a)) in nir_opt_algebraic.py rewrote a down-cast of a sign-extended value as i2iN(a), whatever the size of a. When a is wider than N that is an i2i down-cast, which nir_validate rejects (an i2i source must not be wider than the result). u2u8(i2i32(a@16)) became i2i8(a@16), and the shader failed validation right after nir_opt_algebraic.

This change uses the u2uN form for down-casts, which is what upstream main has:

• u2uN(i2iM(a@P)) is u2uN(a) when P is at least N, and i2iN(a) when P is smaller than N, where it is an up-cast that keeps the sign extension.
• u2uN(u2uM(a)) is u2uN(a).

How it showed up

A llama.cpp Vulkan compute shader (the unpack of four int8 values from a sign-extended int16 load) aborted at pipeline compile on the G17C driver with NIR validation failed after nir_opt_algebraic, 84 errors src_bit_size <= instr->def.bit_size. Tracing the rule matcher on the same NIR printed (u2u8 (i2i32 a)) -> (i2i8 a).

Test

nir_opt_algebraic_test.downcast_of_sign_extended_value_is_u2u builds the same pattern (16-bit global load, i2i32, u2u8) and runs nir_opt_algebraic.

• Without the change: fails, one i2i8 narrows a 16-bit value.
• With the change: passes.
• The whole nir_tests suite passes, 6246 tests, including the generated algebraic pattern tests.
• agx_tests on the same tree: 497 passed, 1 skipped (the existing NativeFMA.ExactDyadicOracle skip).

Hardware: on an M5 Max with this change in a private build, the llama.cpp Vulkan load no longer aborts in nir_opt_algebraic. That run then stops at a separate assertion in agx_nir_lower_cmat.c (cooperative matrix multiply-add types), which this PR does not touch.

[greptile-apps]

Confidence Score: 5/5

[Medium impact] The PR appears safe to merge.

Summary

Fixes integer cast folding in nir_opt_algebraic so narrowing a sign-extended value does not produce an invalid signed down-cast.

• Nested integer casts keep down-casts valid.

No actionable issues were found. Tests were not run during this review.

_{Reviews (1) · Last reviewed commit: "nir/algebraic: keep down-casts of signed..." · Reviewed by Greptile}

[maralcbr]

Thanks @joshuaswarren. The diagnosis is right, and as far as I can tell by reading, the new loop gets every case right. No code blockers.

I checked nir_opt_algebraic.py:3029-3052 against nir_validate.c:289-297. No rule emits a narrowing i2i any more. u2uN(i2iM(a@P)) becomes u2uN(a) when P >= N and i2iN(a) when P < N, which keeps the sign extension. u2uN(u2uM(a)) becomes u2uN(a), which holds for any width of a. Skipping P > M means the rules never match an invalid inner cast. The new test is worth having: the generated pattern tests build both sides of each rule through the nir_i2iN builders, and those already turn a narrowing i2i into u2u (nir_builder_opcodes_h.py:49-56), so those tests could never see this bug.

Before merge:

• CI hasn't run the NIR tests yet. Both build jobs are pending and ci / result isn't reported. plan and source passed, but source only runs lint, the kernel-ABI checks and the Asahi CPU fixtures, not nir_tests or the generated pattern tests.

Smaller things:

• Please cover the sign-extension branch too. algebraic_tests.cpp:1355-1370 only exercises P >= N, and it only asserts that no narrowing i2i is left. Could you assert that the result is u2u8 of the 16-bit load? Could you also add a P < N case, such as u2u16(i2i32(a@8)), that expects i2i16 of the load? That branch (nir_opt_algebraic.py:3048) has no hand-written test right now.
• Please name the upstream commit in the commit message. I can't confirm "as upstream does" from here, and upstream 52d002838da is already in this file's history. If it's a fork-local fix, saying so would make the next upstream import easier.

Nits, fine as follow-ups:

• The N == 1 branch no longer folds i2i1(y2yM(a)). nir_validate.c:293-296 doesn't restrict i2i1, and DXIL consumes it (nir_to_dxil.c:2358). The builder already turns a narrowing i2i1 into u2u1, so the impact looks small, but please confirm it's intended.
• nir_opt_algebraic.py:3031: suggest "Down-casts are always in the u2uN form; nir_validate rejects narrowing i2iN."

This is shared NIR code that every driver uses, so whether to carry it as a fork fix or take the upstream commit is Marcelo's call. For release, it still needs edge qualification on the affected Macs at the exact commit (AGENTS.md:18-20). The M5 Max run in the body then stops at a separate agx_nir_lower_cmat.c assertion, so llama.cpp on G17C isn't shown working end to end yet.

Reviewed at 94317974785d.

— Marcelo's agent

[maralcbr]

This only checks that no bad cast is left. Could it also assert that the value reaching i2f32 is u2u8 of the 16-bit load, and could a second case cover P < N, for example u2u16(i2i32(a@8)) becoming i2i16?

— Marcelo's agent

[maralcbr]

nit: the old loop also folded i2i1(y2yM(a)), and nir_validate still allows i2i1. Please confirm dropping it is intended.

— Marcelo's agent