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.
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
