> 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? 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. > 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? -- Regards Robin
