On Sun, Jul 26, 2026 at 11:16 AM Rohith Kapelli <[email protected]> wrote: > > simplify_ternary_operation folds IF_THEN_ELSE (cond, a, a) to a using > rtx_equal_p, which ignores the memory attributes. Two loads from the > same address into the same register are rtx_equal_p even when they have > incompatible alias sets, so the fold returns one arm's MEM and the > result inherits just that arm's (too narrow) alias set. > > noce_try_ifelse_collapse builds exactly such an IF_THEN_ELSE from the two > sides of > > long f (int a, void *cc, long *d) > { > long long c; > *d = 0; > if (a) c = *(long *) cc; else c = *(long long *) cc; > *d = 1; > return c; > } > > so ce1 replaced the two loads with a single long long load; a later pass, > seeing that a long long load does not alias the long store to *d, deleted > the "dead" *d = 0, and with cc == d the function returned a stale value. > > The if-conversion shortcut in noce_process_if_block already guards the > analogous move with rtx_interchangeable_p, which for MEMs additionally > requires the memory attributes to match (r213970, PR62004/PR62030). But > that shortcut does not go through simplify_rtx, whereas > noce_try_ifelse_collapse does, so guard the fold itself: only replace the > if-then-else with one operand when they are the same rtx, are not memory, > or have equal memory attributes. This fixes every caller of the rule > rather than one, and matches the interchangeability test used elsewhere. > > Refusing the fold leaves the two identical loads, which are then commoned > by the following cleanup_cfg cross-jump; that runs merge_memattrs and > widens the alias set to 0, so the code is still a single load and is > correct. Ordinary folds -- registers, constants, and MEMs with matching > attributes -- are unchanged, and the generated code is identical to > guarding noce_try_ifelse_collapse directly. > > Various tree passes can factor the two loads with a conservative type > before RTL and so hide this: PRE and code hoisting on the release > branches, and the phi-opt load factoring (PR125557) on trunk. The test > disables them so the if-conversion path is exercised on every affected > version. It is a live wrong-code at -O2 on the 13/14/15/16 branches -- > gcc-13 miscompiles the reduced case at plain -O2 with no such flags at > all. > > PR rtl-optimization/125683 > > gcc/ChangeLog: > > * simplify-rtx.cc (simplify_context::simplify_ternary_operation): > Do not fold an IF_THEN_ELSE of two equal operands when they are > MEMs with different memory attributes.
Can we zero out the memory attributes if they are different (and not a BLK mode)? I think that will work and we still get the correct behavior and the optimization too. Thanks, Andrea > > gcc/testsuite/ChangeLog: > > * gcc.dg/pr125683.c: New test. > > Signed-off-by: Rohith Kapelli <[email protected]> > --- > > Notes for the list (not part of the commit message): > > I first fixed this in noce_try_ifelse_collapse, mirroring the > rtx_interchangeable_p guard that the neighbouring "A and B are really the > same" shortcut in noce_process_if_block has used since r213970 > (PR62004/PR62030). I enumerated every caller of the IF_THEN_ELSE rule > (combine, cse, cselib, recog, cfgexpand, optabs) and confirmed > noce_try_ifelse_collapse is the only one that forms an IF_THEN_ELSE of two > MEMs -- with if-conversion disabled nothing else merges the loads, and > combine's cmoves select registers, never two MEMs -- so an ifcvt-local > guard is complete today. I still prefer guarding the rule: it is correct > by construction for every present and future caller, it cannot be bypassed > by the next new caller the way the 2014 guard was, and this genuinely is a > simplify-rtx unsoundness. The generated code is identical either way. If > you would rather keep the change inside if-conversion, I have that version > ready too. > > Bootstrapped and regression tested on aarch64-linux-gnu, before/after in > the same environment, with no new failures. The revert-sensitivity is by > execution with the two native compilers from that build: the unfixed > compiler's binary aborts (f() drops the *d = 0 store, so > f (1, &storage, &storage) returns a stale value), the patched one returns > 0. The patch applies cleanly to the 13, 14, 15 and 16 branches. > > gcc/simplify-rtx.cc | 11 ++++++++-- > gcc/testsuite/gcc.dg/pr125683.c | 38 +++++++++++++++++++++++++++++++++ > 2 files changed, 47 insertions(+), 2 deletions(-) > create mode 100644 gcc/testsuite/gcc.dg/pr125683.c > > diff --git a/gcc/simplify-rtx.cc b/gcc/simplify-rtx.cc > index 882a11c5760..da7a90f58d4 100644 > --- a/gcc/simplify-rtx.cc > +++ b/gcc/simplify-rtx.cc > @@ -7635,8 +7635,15 @@ simplify_context::simplify_ternary_operation (rtx_code > code, machine_mode mode, > if (CONST_INT_P (op0)) > return op0 != const0_rtx ? op1 : op2; > > - /* Convert c ? a : a into "a". */ > - if (rtx_equal_p (op1, op2) && ! side_effects_p (op0)) > + /* Convert c ? a : a into "a". Beware that two rtx_equal_p MEMs can > + still carry different memory attributes, in particular incompatible > + alias sets; returning one of them would narrow the aliasing of the > + result to that operand's, which is unsound (PR125683). Only fold > + when the operands are truly interchangeable. */ > + if (rtx_equal_p (op1, op2) && ! side_effects_p (op0) > + && (op1 == op2 > + || !MEM_P (op1) > + || mem_attrs_eq_p (get_mem_attrs (op1), get_mem_attrs (op2)))) > return op1; > > /* Convert a != b ? a : b into "a". */ > diff --git a/gcc/testsuite/gcc.dg/pr125683.c b/gcc/testsuite/gcc.dg/pr125683.c > new file mode 100644 > index 00000000000..1f5706261b8 > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/pr125683.c > @@ -0,0 +1,38 @@ > +/* PR rtl-optimization/125683 */ > +/* { dg-do run } */ > +/* { dg-options "-O2 -fno-tree-pre -fno-code-hoisting -fdisable-tree-phiopt1 > -fdisable-tree-phiopt2 -fdisable-tree-phiopt3 -fdisable-tree-phiopt4 > -fdisable-tree-cselim" } */ > + > +/* if-conversion (ce1) used to collapse the two conditional loads below > + into a single load through simplify_gen_ternary's > + (if_then_else c X X) -> X rule, which ignores the memory attributes. > + The two loads have incompatible alias sets, so the collapsed load kept > + just one of them; a later pass then treated it as not aliasing the > + long store to *d and moved it, giving the wrong value when cc == d. > + Several tree passes (PRE/code-hoisting on the release branches, phi-opt > + load factoring on trunk) can factor the two loads with a conservative > + type before RTL and so hide the bug; they are disabled here so the > + if-conversion path is exercised on every affected version. */ > + > +long __attribute__ ((noipa)) > +f (int a, void *cc, long *d) > +{ > + long long c; > + *d = 0; > + if (a) > + c = *(long *) cc; > + else > + c = *(long long *) cc; > + *d = 1; > + return c; > +} > + > +int > +main (void) > +{ > + long storage = -1; > + /* cc == d, and a != 0 so the taken load reads *(long *)cc, which is a > + type-compatible access to the object *d that was just set to 0. */ > + if (f (1, &storage, &storage) != 0) > + __builtin_abort (); > + return 0; > +} > -- > 2.53.0 >
