On 10/23/26 1:22 PM, Vineet Gupta 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. 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]>
And just in case, the functionally equivalent orig v5 which did this post-split is attached here for comparison. It is more code and more involved logic so not really recommended but posted just FYI.
Thx, -Vineet
--- 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.cnew 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; +}
From: Vineet Gupta <[email protected]> Date: Sat, 3 Oct 2026 01:36:09 -0700 Subject: [do-not-merge] combine: Keep volatile reads separate and in order [PR127851] 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. can_combine_p only looks at the insns between INSN and I3, and the result of a split is not known there. So check the result instead: at the start of try_combine, note which volatile MEMs each of I0..I3 reads, and before accepting the new patterns, require that each of NEWPAT and NEWI2PAT reads MEMs from at most one of those insns, and that NEWI2PAT's come from an earlier insn than NEWPAT's. A MEM that cannot be traced to an original insn refuses the combination. 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 (collect_volatile_reads): New function. (volatile_reads_origin): New function. (try_combine): Refuse a combination that would put volatile reads from different insns into one insn, or reorder them. 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. * 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: - Check the new patterns in try_combine instead of I3 in can_combine_p. 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). Dropping the succ == 0 guard instead rejects the valid split in gcc.target/i386/pr122343-4a.c (sashiko). - volatile_reads_p is gone; a bit-field store to a volatile MEM now counts 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 | 84 +++++++++++++++++++ .../aarch64/volatile-fuse-order-1.c | 24 ++++++ .../aarch64/volatile-fuse-order-2.c | 23 +++++ .../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 ++++++++ 8 files changed, 342 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 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..6721d30ce12b 100644 --- a/gcc/combine.cc +++ b/gcc/combine.cc @@ -2508,6 +2508,60 @@ count_auto_inc (rtx, rtx, rtx, rtx, rtx, void *arg) return 0; } +/* Add to V each volatile MEM that insn pattern X reads. */ + +static void +collect_volatile_reads (rtx x, vec<rtx> *v) +{ + subrtx_var_iterator::array_type array; + FOR_EACH_SUBRTX_VAR (iter, array, x, NONCONST) + if (MEM_P (*iter) && MEM_VOLATILE_P (*iter)) + v->safe_push (*iter); + + /* 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. */ + if (asm_noperands (x) >= 0) + return; + int n = GET_CODE (x) == PARALLEL ? XVECLEN (x, 0) : 1; + for (int i = 0; i < n; i++) + { + rtx elt = GET_CODE (x) == PARALLEL ? XVECEXP (x, 0, i) : x; + if ((GET_CODE (elt) == SET || GET_CODE (elt) == CLOBBER) + && MEM_P (SET_DEST (elt))) + for (unsigned j = 0; j < v->length (); j++) + if ((*v)[j] == SET_DEST (elt)) + { + v->unordered_remove (j); + break; + } + } +} + +/* Return which of the insns in ORIG all the volatile MEMs read by PAT + came from, -1 if PAT reads none, or -2 if they came from more than one + or from none of them, such as a copy. */ + +static int +volatile_reads_origin (rtx pat, const auto_vec<rtx> orig[4]) +{ + auto_vec<rtx> v; + collect_volatile_reads (pat, &v); + int origin = -1; + for (rtx mem : v) + { + int k = 0; + while (k < 4 && !orig[k].contains (mem)) + k++; + if (k == 4 || (origin >= 0 && origin != k)) + return -2; + origin = k; + } + return origin; +} + /* 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 +2758,18 @@ 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); + /* The volatile MEMs read by I0, I1, I2 and I3, before any substitution, + and how many of those insns read one. */ + auto_vec<rtx> vol_reads[4]; + int n_vol_insns = 0; + rtx_insn *orig_insns[4] = { i0, i1, i2, i3 }; + for (int k = 0; k < 4; k++) + if (orig_insns[k]) + { + collect_volatile_reads (PATTERN (orig_insns[k]), &vol_reads[k]); + n_vol_insns += !vol_reads[k].is_empty (); + } + added_links_insn = 0; added_notes_insn = 0; @@ -4136,6 +4202,24 @@ try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn *i1, rtx_insn *i0, } } + /* Volatile reads from different insns must stay in different insns, + in their original order. NEWI2PAT goes before NEWPAT. This also + catches a split that reassociated the reads, e.g. (a + x) + y into + (a + y) + x, and then put y into NEWI2PAT. */ + if (n_vol_insns > 1) + { + int o3 = volatile_reads_origin (newpat, vol_reads); + int o2 = newi2pat ? volatile_reads_origin (newi2pat, vol_reads) : -1; + if (o3 == -2 || o2 == -2 || (o2 >= 0 && o3 >= 0 && o2 >= o3)) + { + if (dump_file && (dump_flags & TDF_DETAILS)) + fprintf (dump_file, + "Volatile reads would be merged or reordered\n"); + undo_all (); + return 0; + } + } + /* Reject this combination if insn_cost reports that the replacement instructions are more expensive than the originals. */ if (!combine_validate_cost (i0, i1, i2, i3, newpat, newi2pat, other_pat, 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..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
