Hi Robin, > On 16 Jul 2026, at 10:09, Robin Dapp <[email protected]> wrote: > >> init_noce_multiple_sets_info records earlier SET destinations that are >> mentioned by a later SET source. When a pseudo is set more than once, only >> its most recent prior definition reaches that source. >> >> Recording every definition is unsafe in the second >> noce_convert_multiple_sets_1 attempt. The newest definition can be emitted >> directly into its target pseudo, making its replacement a no-op. A later >> replacement using an older definition then substitutes a stale value. >> >> PR126184 contains the following dependency chain: >> >> c = x + 1; >> x = c * y; >> c = z + 3; >> y = c * x; >> >> The unfixed conversion computes the last multiply with the temporary that >> holds `x + 1`, rather than the reaching `z + 3` value. This makes the >> testcase abort on AArch64. >> >> The unfixed final sequence forms `x + 1` in x0 and later uses x0 as the >> multiply operand: >> >> add x0, x1, 1 >> csel x1, x1, x3, eq >> mul x0, x1, x0 >> >> With the fix, x3 retains z until `z + 3` is formed and used by the multiply: >> >> add x3, x3, 3 >> csel x0, x0, x1, ne >> mul x3, x0, x3 >> >> Walk definitions from newest to oldest and record only the first match for >> each pseudo. Restrict this conversion to pseudo destinations. Hard >> registers can have overlapping definitions in different modes, and an exact >> RTL replacement cannot represent the value produced by a partial or >> mode-changing definition. > > While the hard-reg issue seems real and latent, how did it even come up? Did > you see it in the aarch64 test of the PR? I guess not as you're adding a > separate test for it? It looks exceedingly rare to me, are we sure the rest > of > ifcvt is safe WRT hard regs?
The PR testcase only uses pseudo destinations, so it did not expose the hard-register case. I found that during a separate audit of the dependency rewiring. reg_mentioned_p and the deduplication bitmap identify registers by register number, whereas simplify_replace_rtx only replaces an rtx_equal_p match. Different-mode definitions of overlapping hard registers can therefore be treated as the same dependency even though the later replacement cannot represent them as the same value. That is why I added a separate RTL testcase. I also got a notification from the Linaro CI when it tested my patch at: https://gcc.gnu.org/pipermail/gcc-patches/2026-July/724006.html Without this fix it hit the !HARD_REGISTER_P (SET_DEST (set)) during arm bootstrap so this case can be hit apparently unless this patch rejects hard registers in bb_ok_for_noce_convert_multiple_sets > > While the current fix looks reasonable to me, did you consider a > dataflow-based > approach? It could perhaps help with both issues at once? On the other > hard, > it might not be worth it at all, given how rare hard regs are here. At ce1 we have scan and liveness information, but not reaching definitions or UD chains. Adding a UD chain would add the reaching definitions problem for a query confined to one straight-line block. Here every candidate is a validated single SET, only definitions within the same arm can be rewired, and the block size is bounded by max-rtl-if-conversion-insns. Walking earlier SETs backwards and taking the first matching pseudo is therefore the local reaching-definition computation that the transform needs. It felt too heavyweight for this problem > >> dests.safe_push (dest); >> @@ -4270,10 +4272,12 @@ bb_ok_for_noce_convert_multiple_sets (basic_block >> test_bb, unsigned *cost) >> rtx dest = SET_DEST (set); >> rtx src = SET_SRC (set); >> >> - /* Do not handle anything involving memory loads/stores since it might >> - violate data-race-freedom guarantees. Make sure we can force SRC >> + /* Dependency rewiring requires pseudo destinations with one fixed >> + mode. Do not handle anything involving memory loads/stores since it > > "Pseudo destinations with one fixed mode" sounds slightly weird, they always > have one mode? Yeah agreed, I’ll reword it. Thanks, Kyrill > > -- > Regards > Robin >
