[llvm] [X86] Prevent NUW flag forwarding in ADD->SUB peephole (PR #201612)

via llvm-commits llvm-commits at lists.llvm.org
Sun Sep 13 09:35:19 PDT 2026


hesam-oxe wrote:

@topperc @RKSimon Agreed on the direction — the general mechanism is the right fix, and I'm happy to take the yak on. 🙂

Two updates on the branch while that discussion settles:

1. **Pre-merge failure fixed** (`2aec24c1`): the `add %r8, 128` in `avx512vnni-combine.ll` carries NUW (it comes out of the vnni combining), so with `add_without_nuw` it correctly no longer converts to `subq $-128` — the expectation is now `addq $128, %r8`. I also dropped a stray whitespace-only line the original patch had introduced next to the `INSERT_SUBREG` patterns.
2. I could not run a full LLVM build locally for this update; the pre-merge CI on the new head will validate it.

On the **general mechanism**: as I understand the current plumbing, poison-generating flags reach the MIR `SUB` because ISel copies the matched SDNode's flags onto the selected `MachineInstr` without asking whether the pattern preserves the operation's semantics — and ADD→SUB with a negated immediate is exactly a case where downstream MIR transforms shouldn't inherit the ADD's flag reasoning. Sketch of what I'd propose:

- In the flag-transfer path (`SelectionDAGISel`), **don't copy `nsw`/`nuw`/`exact`/`inbounds`/`disjoint` by default** when the selected opcode differs from the matched one (or more conservatively: by default, period).
- Add an **opt-in marker** for patterns where flag transfer is provably semantics-preserving — e.g. a TableGen `let CopyPoisonFlags = 1` on the pattern (similar in spirit to `SDNP*` properties), so each target has to claim flag preservation explicitly rather than getting it silently.
- That fixes the whole class across targets, not just the ~20 X86 patterns guarded by `add_without_nuw` here, and matches what @arsenm/@RKSimon raised on #160217.

**Question for you both:** should this PR (a) stay as the targeted X86 stopgap until the general mechanism lands — it does fix the concrete miscompile from #160217 today — or (b) be retooled into the general mechanism directly? Happy to do either; if (b), I'd de-scope the `.td` changes here and build the opt-in plumbing in a fresh PR.

https://github.com/llvm/llvm-project/pull/201612


More information about the llvm-commits mailing list