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 returned one arm's MEM and the
result inherited 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.
Rather than dropping the fold when the attributes differ, keep it and
return a reference that only claims what both operands guarantee, in the
spirit of merge_memattrs: alias set 0 when the sets differ, MEM_EXPR and
offset cleared when they disagree, and the minimum alignment. The
operands can be shared, so the attributes are set on a shallow copy
rather than in place.
Unlike merge_memattrs, which fixes up two references that both remain in
the instruction stream, this returns a single reference standing in for
either arm, so the size is kept only when both agree instead of taking
the larger one. BLKmode is left alone because there MEM_ATTRS describes
the size of the access itself, and the fold is also skipped when the two
operands disagree about volatility. Address spaces need no check:
rtx_equal_p already fails for MEMs in different address spaces.
Several 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):
When folding an IF_THEN_ELSE of two equal MEM operands with
different memory attributes, return a copy whose attributes are
widened to what both operands allow.
gcc/testsuite/ChangeLog:
* gcc.dg/pr125683.c: New test.
Signed-off-by: Rohith Kapelli <[email protected]>
---
> 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.
That works, and it keeps the optimization. v2 below does it that way.
When the two operands are rtx_equal_p MEMs whose attributes differ, and
the mode is not BLKmode, the fold now returns a shallow_copy_rtx of one
of them with the attributes widened the way merge_memattrs does: alias
set 0 when the sets differ, MEM_EXPR and offset cleared when they
disagree, MIN alignment. The copy is needed because the operands can be
shared and set_mem_alias_set and friends modify in place. Size is kept
only when both operands agree rather than taking the larger one, since
unlike merge_memattrs -- which fixes up two references that both stay in
the stream -- this returns one reference standing in for either arm. I
also skip the fold when the operands disagree about MEM_VOLATILE_P,
because rtx_equal_p does not compare that flag; address spaces need no
check, since rtx_equal_p already fails for MEMs in different address
spaces.
On the testcase ce1 now reports
if-conversion succeeded through noce_try_ifelse_collapse
and the surviving insn is a single load with the conservative
attributes:
(insn 39 ... (set (reg:DI 101)
(mem:DI (reg/v/f:DI 103 [ cc ]) [0 S8 A64])))
whereas v1 declined the fold ("0 IF blocks converted") and the two loads
were only commoned later by cleanup_cfg cross-jumping. So this keeps the
transformation in the pass instead of relying on a later one.
Bootstrapped and regression tested on aarch64-linux-gnu from a clean
build (stage2 == stage3): no PASS->FAIL in gcc, g++ or libstdc++ against
the unpatched baseline, and gcc.dg/pr125683.c passes. (One caveat worth
passing on: a tree that has been rebuilt incrementally over an earlier
bootstrap can still hold precompiled headers from the older compiler, and
that produced failures here which disappear in a from-scratch build.)
Thanks for the suggestion -- this shape is better than what I posted.
Rohith
gcc/simplify-rtx.cc | 48 +++++++++++++++++++++++++++++++--
gcc/testsuite/gcc.dg/pr125683.c | 38 ++++++++++++++++++++++++++
2 files changed, 84 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..1e436bd7648 100644
--- a/gcc/simplify-rtx.cc
+++ b/gcc/simplify-rtx.cc
@@ -7635,9 +7635,53 @@ 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". */
+ /* 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). When the
+ attributes differ, fold to a copy that keeps only what both operands
+ guarantee, like merge_memattrs does when cross-jumping commons two
+ memory references. */
if (rtx_equal_p (op1, op2) && ! side_effects_p (op0))
- return op1;
+ {
+ if (op1 == op2
+ || !MEM_P (op1)
+ || mem_attrs_eq_p (get_mem_attrs (op1), get_mem_attrs (op2)))
+ return op1;
+
+ /* For BLKmode the size in MEM_ATTRS describes the access itself,
+ so it cannot be dropped; likewise do not lose volatility. */
+ if (GET_MODE (op1) != BLKmode
+ && MEM_VOLATILE_P (op1) == MEM_VOLATILE_P (op2))
+ {
+ rtx mem = shallow_copy_rtx (op1);
+
+ if (MEM_ALIAS_SET (op1) != MEM_ALIAS_SET (op2))
+ set_mem_alias_set (mem, 0);
+
+ if (!mem_expr_equal_p (MEM_EXPR (op1), MEM_EXPR (op2)))
+ {
+ set_mem_expr (mem, NULL_TREE);
+ clear_mem_offset (mem);
+ }
+ else if (MEM_OFFSET_KNOWN_P (op1) != MEM_OFFSET_KNOWN_P (op2)
+ || (MEM_OFFSET_KNOWN_P (op1)
+ && maybe_ne (MEM_OFFSET (op1), MEM_OFFSET (op2))))
+ clear_mem_offset (mem);
+
+ /* Unlike merge_memattrs, which fixes up two references that
+ both stay in the stream, this returns a single reference
+ that stands in for either arm, so keep the size only when
+ both agree rather than taking the larger one. */
+ if (!MEM_SIZE_KNOWN_P (op1) || !MEM_SIZE_KNOWN_P (op2)
+ || maybe_ne (MEM_SIZE (op1), MEM_SIZE (op2)))
+ clear_mem_size (mem);
+
+ set_mem_align (mem, MIN (MEM_ALIGN (op1), MEM_ALIGN (op2)));
+
+ return mem;
+ }
+ }
/* Convert a != b ? a : b into "a". */
if (GET_CODE (op0) == NE
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
