On Fri, Oct 23, 2026 at 10:22 PM Vineet Gupta <[email protected]> wrote: > > 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.
But the result is still better than the code without -ffuse-ops-with-volatile-access. > 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. The approach LGTM (but I can't approve the patch) with minor points below. So, LGTM from the x86 side (tests and x86 interaction): Acked-by: Uros Bizjak <[email protected]> > 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. */ The justification, that an insn does all its reads before its writes, only holds when the original order was read, then store. If I2 stores to volatile y and I3 reads volatile x, fusing them into one insn would reorder the accesses. This is probably unreachable through LOG_LINKS, since a plain store has no register dest to link on. A PARALLEL I2 that sets flags and a volatile MEM could get there. It deserves either a short comment explaining why it can’t happen. > + 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; > + } > + } try_combine is very hot, and the new check walks every sub-rtx of up to four patterns on every call, before most of the cheap rejections. Bail out as soon as the count reaches 2, since the loop currently walks all four insns regardless. Also, please use ARRAY_SIZE macro. { rtx_insn *insns[] = { i0, i1, i2, i3 }; int n_vol_insns = 0; for (unsigned k = 0; k < ARRAY_SIZE (insns); k++) if (insns[k] && volatile_reads_p (PATTERN (insns[k])) && ++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; } } If, as the commit message says, these combinations only arise with -ffuse-ops-with-volatile-access, gate the whole block on flag_fuse_ops_with_volatile_access. Uros. > + > 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 >
