On 10/21/26 2:17 PM, Vineet Gupta wrote:
With -ffuse-ops-with-volatile-access general_operand accepts volatile
MEMs during combine. For "int a = x; int b = y; return a < b;" with
volatile x and y, combine first merges the load of y into the compare
and then the load of x, giving (compare (mem/v x) (mem/v y)). The
operands of one insn are unordered, so whatever later reloads or splits
it picks the order of the two volatile reads, and the sequence point
between them is lost. can_combine_p checks insns between INSN and I3
for volatile references, but not I3 itself.
The x86 compare patterns now reject such operands (dc57b45e7a79), but
other patterns accept them too: for "h = hi; l = lo;" combined into
((dw) h << N) | l, the x86 doubleword concat patterns take both halves
as memory operands and their split reads lo first. Rather than fixing
each pattern, refuse in combine to combine an insn that reads volatile
memory directly into an I3 that already reads volatile memory.
The new volatile_reads_p walks I3's pattern: every SET in a PARALLEL
counts, and so does anything read to form a stored location (a MEM's
address, a ZERO_EXTRACT's position and size), but a volatile MEM that is
only stored to does not, since an insn does all its reads before its
writes. That does not hold for an asm, whose template may access its
operands in any order, so for an asm I3 any volatile reference refuses.
Bootstrapped and regression tested on x86_64-linux-gnu and aarch64-linux-gnu.
PR rtl-optimization/127851
gcc/ChangeLog:
* combine.cc (volatile_reads_p): New function.
(can_combine_p): Use it to refuse combining an insn that reads
volatile memory directly into an I3 that already reads volatile
memory; for an asm I3, refuse on any volatile reference.
gcc/testsuite/ChangeLog:
* gcc.target/aarch64/volatile-fuse-order-1.c: New test.
* gcc.target/aarch64/volatile-fuse-order-2.c: New test.
* gcc.target/i386/volatile-fuse-order-4.c: New test.
* gcc.target/i386/volatile-fuse-order-5.c: New test.
* gcc.target/i386/volatile-fuse-order-6.c: New test.
Signed-off-by: Vineet Gupta <[email protected]>
---
v4: - volatile_reads_p walks I3's pattern: every SET in a PARALLEL counts,
and so do reads in a SET's destination (a MEM's address, a
ZERO_EXTRACT's position/size); a volatile store alone does not
(Jeff).
- For an asm I3 any volatile reference refuses: its template may write
an output before reading an input.
- x86 volatile-fuse-order-1/2/3 dropped, they went in with
dc57b45e7a79. New x86 tests: -4/-5 (asm, including multi-output
PARALLELs) and -6 (doubleword concat), which still fail on trunk.
- With the i386.md part of dc57b45e7a79 reverted, -1..-6 still pass:
combine refuses the merge itself.
v3: the x86 check-function-bodies now match only the load and the compare;
use ia32 / ! ia32 selectors, so -mx32 is covered too (Uros).
v2: the x86 check-function-bodies tests now also run and check the bodies
on ia32 (Uros); added -fno-pic for the global-variable ones.
gcc/combine.cc | 58 +++++++++++++++++++
.../aarch64/volatile-fuse-order-1.c | 24 ++++++++
.../aarch64/volatile-fuse-order-2.c | 23 ++++++++
.../gcc.target/i386/volatile-fuse-order-4.c | 49 ++++++++++++++++
.../gcc.target/i386/volatile-fuse-order-5.c | 45 ++++++++++++++
.../gcc.target/i386/volatile-fuse-order-6.c | 35 +++++++++++
6 files changed, 234 insertions(+)
create mode 100644 gcc/testsuite/gcc.target/aarch64/volatile-fuse-order-1.c
create mode 100644 gcc/testsuite/gcc.target/aarch64/volatile-fuse-order-2.c
create mode 100644 gcc/testsuite/gcc.target/i386/volatile-fuse-order-4.c
create mode 100644 gcc/testsuite/gcc.target/i386/volatile-fuse-order-5.c
create mode 100644 gcc/testsuite/gcc.target/i386/volatile-fuse-order-6.c
diff --git a/gcc/combine.cc b/gcc/combine.cc
index c9daefc905c3..b369914e49e4 100644
--- a/gcc/combine.cc
+++ b/gcc/combine.cc
@@ -1723,6 +1723,53 @@ set_nonzero_bits_and_sign_copies (rtx x, const_rtx set,
void *data)
}
}
+/* Return true if insn pattern X reads volatile memory. */
+
+static bool
+volatile_reads_p (const_rtx x)
+{
+ switch (GET_CODE (x))
+ {
+ case PARALLEL:
+ /* Every element counts, not just the one single_set would pick. */
+ for (int i = 0; i < XVECLEN (x, 0); i++)
+ if (volatile_reads_p (XVECEXP (x, 0, i)))
+ return true;
+ return false;
+
+ case SET:
+ {
+ /* Strip down to the stored location. The position and size of a
+ bit-field are read. */
+ rtx dest = SET_DEST (x);
+ while (GET_CODE (dest) == SUBREG
+ || GET_CODE (dest) == STRICT_LOW_PART
+ || GET_CODE (dest) == ZERO_EXTRACT)
+ {
+ if (GET_CODE (dest) == ZERO_EXTRACT
+ && (volatile_refs_p (XEXP (dest, 1))
+ || volatile_refs_p (XEXP (dest, 2))))
+ return true;
+ dest = XEXP (dest, 0);
+ }
+ /* A stored MEM is not read, and since an insn does all its reads
+ before its writes, a volatile store cannot move ahead of a
+ volatile read in the same insn. Its address is read, though,
+ e.g. (mem (mem/v ...)). */
+ if (MEM_P (dest) && volatile_refs_p (XEXP (dest, 0)))
+ return true;
+ return volatile_refs_p (SET_SRC (x));
+ }
+
+ case CLOBBER:
+ /* A clobbered MEM is not read, but its address is. */
+ return MEM_P (XEXP (x, 0)) && volatile_refs_p (XEXP (XEXP (x, 0), 0));
+
+ default:
+ return volatile_refs_p (x);
+ }
+}
+
/* See if INSN can be combined into I3. PRED, PRED2, SUCC and SUCC2 are
optionally insns that were previously combined into I3 or that will be
combined into the merger of INSN and I3. The order is PRED, PRED2,
@@ -2004,6 +2051,17 @@ can_combine_p (rtx_insn *insn, rtx_insn *i3, rtx_insn
*pred ATTRIBUTE_UNUSED,
return false;
if (succ != 0 && volatile_refs_p (PATTERN (succ)))
return false;
+ /* Nor may INSN go straight into an I3 that already reads volatile
+ memory: the insn would hold both reads with no order between them.
+ A volatile store in I3 is fine, since all of an insn's reads happen
+ before its writes, but not in an asm, whose template may access its
+ operands in any order. (When INSN goes through SUCC, combine may
+ split the result back into two insns that keep the reads ordered.) */
+ if (succ == 0
+ && (asm_noperands (PATTERN (i3)) >= 0
+ ? volatile_refs_p (PATTERN (i3))
+ : volatile_reads_p (PATTERN (i3))))
+ return false;
I don't want to jump the gun on the due review process but seems like
there is some feedback from gcc sashiko which Uroz shared with me [1]
and the issue it found seems legit.
[1]
https://gcc.sashiko.dev/#/patchset/20261021211758.3144964-1-vineet.gupta%40linux.dev
Essentially the "succ == 0" check above implies this only happens for I2
-> I3 combine, but leaves the 3 (or 4) combine cases broken. e.g. Test
below on x86, z is incorrectly read before y (with my v4 and despite
Uroz's i386.md fix)
extern volatile int x, y, z;
int noarg (void) { int t1 = x; int t2 = y; int t3 = z; return t1 +
t2 + t3; }
Dropping the check fixes this test above but regresses other valid
cases: the test from original introduction of
-ffuse-ops-with-volatile-access.
The issue is can_combine_p only able to see the inputs while ensuing
failing to recog and split could re-introduce the reordering. So the fix
is to check later, after any splits (with a pre-scan recording the
volatiles).
Is this something that seems like a right approach for a v5 ?
Thx,
-Vineet
/* We'll check insns between INSN and I3 below. */
}
diff --git a/gcc/testsuite/gcc.target/aarch64/volatile-fuse-order-1.c b/gcc/testsuite/gcc.target/aarch64/volatile-fuse-order-1.c
new file mode 100644
index 000000000000..25d9bf6540e4
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/volatile-fuse-order-1.c
@@ -0,0 +1,24 @@
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+/* { dg-final { check-function-bodies "**" "" "" { target lp64 } } } */
+
+/* The two volatile reads are in separate full expressions, so *x must be
+ read before *y. Combine must not put both reads into one insn, which
+ gives them no order. */
+
+/*
+** f:
+** ldr (w[0-9]+), \[x0\]
+** ldr (w[0-9]+), \[x1\]
+** cmp \1, \2
+** cset w0, lt
+** ret
+*/
+
+int
+f (volatile int *x, volatile int *y)
+{
+ int a = *x;
+ int b = *y;
+ return a < b;
+}
diff --git a/gcc/testsuite/gcc.target/aarch64/volatile-fuse-order-2.c
b/gcc/testsuite/gcc.target/aarch64/volatile-fuse-order-2.c
new file mode 100644
index 000000000000..9a391dc866a0
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/volatile-fuse-order-2.c
@@ -0,0 +1,23 @@
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+/* { dg-final { check-function-bodies "**" "" "" { target lp64 } } } */
+
+/* Both reads of *P are in separate full expressions: the first read must
+ be the first operand of the compare. */
+
+/*
+** f:
+** ldr (w[0-9]+), \[x0\]
+** ldr (w[0-9]+), \[x0\]
+** cmp \1, \2
+** cset w0, lt
+** ret
+*/
+
+int
+f (volatile int *p)
+{
+ int a = *p;
+ int b = *p;
+ return a < b;
+}
diff --git a/gcc/testsuite/gcc.target/i386/volatile-fuse-order-4.c
b/gcc/testsuite/gcc.target/i386/volatile-fuse-order-4.c
new file mode 100644
index 000000000000..d960597f4d31
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/volatile-fuse-order-4.c
@@ -0,0 +1,49 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fomit-frame-pointer -fno-pic -masm=att" } */
+/* { dg-final { check-function-bodies "**" "" "" { target { ! *-*-darwin* } }
} } */
+
+/* The two volatile reads are in separate full expressions, so x must be
+ read before y. Combine must not turn both into memory operands of one
+ asm, whose template may access them in either order. The asm in
+ two_out has two outputs, so it is a PARALLEL of two SETs rather than a
+ single SET. */
+
+/*
+**two_out:
+**...
+** movl x(|\(%rip\)), %e[a-z]+
+**...
+** nop # %e[a-z]+ y(|\(%rip\))
+**...
+*/
+
+/*
+**one_out:
+**...
+** movl x(|\(%rip\)), %e[a-z]+
+**...
+** nop # %e[a-z]+ y(|\(%rip\))
+**...
+*/
+
+extern volatile int x, y;
+
+int
+two_out (void)
+{
+ int a = x;
+ int b = y;
+ int lo, hi;
+ __asm__ ("nop # %2 %3" : "=r" (lo), "=r" (hi) : "rm" (a), "rm" (b));
+ return lo + hi;
+}
+
+int
+one_out (void)
+{
+ int a = x;
+ int b = y;
+ int lo;
+ __asm__ ("nop # %1 %2" : "=r" (lo) : "rm" (a), "rm" (b));
+ return lo;
+}
diff --git a/gcc/testsuite/gcc.target/i386/volatile-fuse-order-5.c
b/gcc/testsuite/gcc.target/i386/volatile-fuse-order-5.c
new file mode 100644
index 000000000000..9ecf2de7d49b
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/volatile-fuse-order-5.c
@@ -0,0 +1,45 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fomit-frame-pointer -fno-pic -masm=att" } */
+/* { dg-final { check-function-bodies "**" "" "" { target { ! *-*-darwin* } }
} } */
+
+/* x is read before the asm that writes volatile z. An ordinary insn does
+ all its reads before its writes, but an asm template may access its
+ operands in any order, so combine must not turn the read of x into a
+ memory operand of that asm. The asm in store_two_out has two outputs
+ (a PARALLEL); the one in store_one_out has one (a single SET). */
+
+/*
+**store_two_out:
+**...
+** movl x(|\(%rip\)), %e[a-z]+
+**...
+** nop # z(|\(%rip\)) %e[a-z]+ %e[a-z]+
+**...
+*/
+
+/*
+**store_one_out:
+**...
+** movl x(|\(%rip\)), %e[a-z]+
+**...
+** nop # z(|\(%rip\)) %e[a-z]+
+**...
+*/
+
+extern volatile int x, z;
+
+int
+store_two_out (void)
+{
+ int a = x;
+ int o;
+ __asm__ ("nop # %0 %1 %2" : "=m" (z), "=r" (o) : "rm" (a));
+ return o;
+}
+
+void
+store_one_out (void)
+{
+ int a = x;
+ __asm__ ("nop # %0 %1" : "=m" (z) : "rm" (a));
+}
diff --git a/gcc/testsuite/gcc.target/i386/volatile-fuse-order-6.c
b/gcc/testsuite/gcc.target/i386/volatile-fuse-order-6.c
new file mode 100644
index 000000000000..9ffa6503e55a
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/volatile-fuse-order-6.c
@@ -0,0 +1,35 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fno-pic -masm=att" } */
+/* { dg-final { check-function-bodies "**" "" "" { target { ! *-*-darwin* } }
} } */
+
+/* hi is read before lo, in separate full expressions. Combine must not
+ put both volatile reads into one insn: the doubleword concat patterns
+ accept both halves as memory operands, and the split then reads them
+ in its own order. */
+
+/*
+**f:
+**...
+** mov(l|q) hi(|\(%rip\)), %[re]?[a-d]x
+** mov(l|q) lo(|\(%rip\)), %[re]?[a-d]x
+**...
+*/
+
+#ifdef __SIZEOF_INT128__
+typedef unsigned __int128 dw;
+typedef unsigned long long hw;
+#else
+typedef unsigned long long dw;
+typedef unsigned int hw;
+#endif
+
+extern volatile hw hi, lo;
+
+dw
+f (dw x)
+{
+ hw h = hi;
+ hw l = lo;
+ dw v = ((dw) h << (8 * sizeof (hw))) | l;
+ return x + v;
+}
