With -ffuse-ops-with-volatile-access general_operand accepts volatile
MEMs during combine, which can then lose the order of volatile reads in
two ways.
It can put two of them into one insn. For "int a = x; int b = y;
return a < b;" combine 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 reads. The x86 compare patterns now reject such operands
(dc57b45e7a79), but e.g. the doubleword concat patterns take both halves
of ((dw) hi << N) | lo as memory operands, and their split reads lo
first.
It can also split a combination back into two insns in the wrong order.
For "t1 = x; t2 = y; t3 = z; return t1 + t2 + t3;" the load of z is
merged into the last add first. Combining the load of y and the add of
y into it gives (t1 + y) + z, which simplify reassociates to (t1 + z) + y;
that does not match, and the split puts t1 + z into the first insn, so
x86 reads x, z, y. gcc.dg/pr56144.c gets the same treatment.
Since a volatile read is never dropped by combining, a combination of
insns of which two or more read volatile memory always puts those reads
either into one insn or into a split whose order is not guaranteed. So
refuse such combinations up front in try_combine, while I0..I3 are
unaltered. can_combine_p already refuses this when INSN goes through an
intermediate insn that reads volatile memory, but not when I3 itself
does.
This also refuses the 3-insn combination in gcc.target/i386/pr122343-4a.c,
whose split happened to keep the reads in order, at the cost of one insn:
"addl bar" becomes a load of bar and a register add.
A MEM that is only stored to is not a read, since an insn does all its
reads before its writes, so "v += 1" can still become one RMW insn.
That does not hold for an asm, whose template may access its operands in
any order, so there a stored MEM counts as a read.
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.
(try_combine): Refuse to combine insns of which more than one reads
volatile memory.
gcc/testsuite/ChangeLog:
* gcc.target/i386/pr122343-4a.c: Expect bar to be loaded into a
register before the add.
* 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.
* gcc.target/i386/volatile-fuse-order-7.c: New test.
* gcc.target/i386/volatile-fuse-order-8.c: New test.
Signed-off-by: Vineet Gupta <[email protected]>
---
v5: - Refuse to combine insns of which more than one reads volatile memory,
checked in try_combine while I0..I3 are unaltered (Jeff, Uros). v4
only checked I3 when INSN went straight into it (succ == 0), assuming
a 3- or 4-insn combination is split back in order. It need not be:
for "t1 = x; t2 = y; t3 = z; return t1 + t2 + t3;" the split reads z
before y, on trunk and v4 (also gcc.dg/pr56144.c) (sashiko).
- That also refuses the split in gcc.target/i386/pr122343-4a.c, which
did keep the order; its expected code now loads bar into a register
before the add. The attached alternative instead checks the new
patterns after any split and keeps that, by tracking each volatile
MEM back to the insn it came from. Same test results otherwise, on
both targets.
- volatile_reads_p now counts a bit-field store to a volatile MEM as a
read (sashiko).
- New x86 tests: -7 (the reassociated split), -8 (RMW still combines);
-6 gains g, a PLUS of the halves, which fails on trunk at -m32
(sashiko). Register regexes in -4/-6 loosened.
- With the i386.md part of dc57b45e7a79 reverted, -1..-8 still pass.
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 | 48 +++++++++++++++++
.../aarch64/volatile-fuse-order-1.c | 24 +++++++++
.../aarch64/volatile-fuse-order-2.c | 23 ++++++++
gcc/testsuite/gcc.target/i386/pr122343-4a.c | 6 ++-
.../gcc.target/i386/volatile-fuse-order-4.c | 50 +++++++++++++++++
.../gcc.target/i386/volatile-fuse-order-5.c | 45 ++++++++++++++++
.../gcc.target/i386/volatile-fuse-order-6.c | 54 +++++++++++++++++++
.../gcc.target/i386/volatile-fuse-order-7.c | 29 ++++++++++
.../gcc.target/i386/volatile-fuse-order-8.c | 33 ++++++++++++
9 files changed, 310 insertions(+), 2 deletions(-)
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
create mode 100644 gcc/testsuite/gcc.target/i386/volatile-fuse-order-7.c
create mode 100644 gcc/testsuite/gcc.target/i386/volatile-fuse-order-8.c
diff --git a/gcc/combine.cc b/gcc/combine.cc
index c9daefc905c3..1198f48cd83c 100644
--- a/gcc/combine.cc
+++ b/gcc/combine.cc
@@ -2508,6 +2508,35 @@ count_auto_inc (rtx, rtx, rtx, rtx, rtx, void *arg)
return 0;
}
+/* Return true if insn pattern X reads volatile memory. */
+
+static bool
+volatile_reads_p (rtx x)
+{
+ /* Only a MEM that is itself the destination of a SET or CLOBBER is not
+ read. Its address still is, and so is a MEM under a ZERO_EXTRACT
+ destination, since a bit-field store keeps the rest of the word. An
+ asm template may access its operands in any order, though, so there a
+ store counts too. */
+ bool is_asm = asm_noperands (x) >= 0;
+ int n = GET_CODE (x) == PARALLEL ? XVECLEN (x, 0) : 1;
+ subrtx_iterator::array_type array;
+ FOR_EACH_SUBRTX (iter, array, x, NONCONST)
+ if (MEM_P (*iter) && MEM_VOLATILE_P (*iter))
+ {
+ bool stored = false;
+ for (int i = 0; i < n && !is_asm && !stored; i++)
+ {
+ rtx elt = GET_CODE (x) == PARALLEL ? XVECEXP (x, 0, i) : x;
+ stored = ((GET_CODE (elt) == SET || GET_CODE (elt) == CLOBBER)
+ && SET_DEST (elt) == *iter);
+ }
+ if (!stored)
+ return true;
+ }
+ return false;
+}
+
/* Try to combine the insns I0, I1 and I2 into I3.
Here I0, I1 and I2 appear earlier than I3.
I0 and I1 can be zero; then we combine just I2 into I3, or I1 and I2 into
@@ -2704,6 +2733,25 @@ try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn *i1,
rtx_insn *i0,
if (i1 && DF_INSN_LUID (i1) > DF_INSN_LUID (i2))
std::swap (i1, i2);
+ /* Refuse to combine insns of which more than one reads volatile memory.
+ Their reads would end up in one insn, which gives them no order, or
+ be split back into two insns in an order nothing guarantees: combine
+ may reassociate (a + x) + y into (a + y) + x and put y first. */
+ {
+ rtx_insn *insns[4] = { i0, i1, i2, i3 };
+ int n_vol_insns = 0;
+ for (int k = 0; k < 4; k++)
+ if (insns[k] && volatile_reads_p (PATTERN (insns[k])))
+ n_vol_insns++;
+ if (n_vol_insns > 1)
+ {
+ if (dump_file && (dump_flags & TDF_DETAILS))
+ fprintf (dump_file, "Can't combine insns that each read volatile "
+ "memory\n");
+ return 0;
+ }
+ }
+
added_links_insn = 0;
added_notes_insn = 0;
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/pr122343-4a.c
b/gcc/testsuite/gcc.target/i386/pr122343-4a.c
index 2426742b6d2d..7d3fe8e0210e 100644
--- a/gcc/testsuite/gcc.target/i386/pr122343-4a.c
+++ b/gcc/testsuite/gcc.target/i386/pr122343-4a.c
@@ -13,7 +13,8 @@
**...
** barrier
**...
-** addl bar(|\(%rip\)), %eax
+** movl bar(|\(%rip\)), %[a-z0-9]+
+** addl %[a-z0-9]+, %eax
** subl bar(|\(%rip\)), %eax
** ret
** .cfi_endproc
@@ -28,7 +29,8 @@
*D...
*D barrier
*D...
-*D addl \(%rdx\), %eax
+*D movl \(%rdx\), %[a-z0-9]+
+*D addl %[a-z0-9]+, %eax
*D subl \(%rdx\), %eax
*D ret
*D...
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..e41889bba06d
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/volatile-fuse-order-4.c
@@ -0,0 +1,50 @@
+/* { 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. y may be loaded into a register or stay a memory operand,
+ but only after x has been loaded. */
+
+/*
+**two_out:
+**...
+** movl x(|\(%rip\)), %[a-z0-9]+
+**...
+** .*[\t ,]y(\(%rip\))?(,.*)?
+**...
+*/
+
+/*
+**one_out:
+**...
+** movl x(|\(%rip\)), %[a-z0-9]+
+**...
+** .*[\t ,]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..930e8426a274
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/volatile-fuse-order-6.c
@@ -0,0 +1,54 @@
+/* { 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. In g the halves meet in a PLUS rather than an IOR. */
+
+/*
+**f:
+**...
+** mov(l|q) hi(|\(%rip\)), %[a-z0-9]+
+**...
+** mov(l|q) lo(|\(%rip\)), %[a-z0-9]+
+**...
+*/
+
+/*
+**g:
+**...
+** mov(l|q) hi(|\(%rip\)), %[a-z0-9]+
+**...
+** (mov|add)(l|q) lo(|\(%rip\)), %[a-z0-9]+
+**...
+*/
+
+#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;
+}
+
+dw
+g (dw x)
+{
+ dw hs = (dw) hi << (8 * sizeof (hw));
+ hw l = lo;
+ dw hsx = hs + x;
+ return hsx + l;
+}
diff --git a/gcc/testsuite/gcc.target/i386/volatile-fuse-order-7.c
b/gcc/testsuite/gcc.target/i386/volatile-fuse-order-7.c
new file mode 100644
index 000000000000..99ba2f551946
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/volatile-fuse-order-7.c
@@ -0,0 +1,29 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fno-pic -masm=att" } */
+/* { dg-final { check-function-bodies "**" "" "" { target { ! *-*-darwin* } }
} } */
+
+/* x, y and z must be read in that order. With z already a memory operand
+ of the last add, combine merged the y load into it, reassociated to
+ (t1 + z) + y, and split that so that z was read before y. */
+
+/*
+**f:
+**...
+** movl x(|\(%rip\)), %[a-z0-9]+
+**...
+** (movl|addl) y(|\(%rip\)), %[a-z0-9]+
+**...
+** addl z(|\(%rip\)), %[a-z0-9]+
+**...
+*/
+
+extern volatile int x, y, z;
+
+int
+f (void)
+{
+ int t1 = x;
+ int t2 = y;
+ int t3 = z;
+ return t1 + t2 + t3;
+}
diff --git a/gcc/testsuite/gcc.target/i386/volatile-fuse-order-8.c
b/gcc/testsuite/gcc.target/i386/volatile-fuse-order-8.c
new file mode 100644
index 000000000000..a565cf49e030
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/volatile-fuse-order-8.c
@@ -0,0 +1,33 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fomit-frame-pointer -fno-pic -masm=att" } */
+/* { dg-final { check-function-bodies "**" "" "" { target { ! *-*-darwin* } }
} } */
+
+/* An insn does all its reads before its writes, so a volatile read may
+ still be combined into a volatile store, and v updated in place. */
+
+/*
+**inc:
+** addl \$1, v(|\(%rip\))
+** ret
+*/
+
+/*
+**add_w:
+** movl w(|\(%rip\)), %[a-z0-9]+
+** addl %[a-z0-9]+, v(|\(%rip\))
+** ret
+*/
+
+extern volatile int v, w;
+
+void
+inc (void)
+{
+ v += 1;
+}
+
+void
+add_w (void)
+{
+ v += w;
+}
--
2.53.0-Meta
