On Wed, Oct 14, 2026 at 11:15 AM Vineet Gupta <[email protected]> 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 and LRA loads y first, so the two > sequenced volatile reads are reordered. can_combine_p checks insns > between INSN and I3 for volatile references, but not I3 itself. > > Refuse to combine an insn that reads volatile memory directly into an > I3 whose source already reads volatile memory. A volatile store in I3 > is still allowed, so read-modify-write forms are unaffected. > > Bootstrapped and regression tested on x86_64-linux-gnu and aarch64-linux-gnu. > > PR rtl-optimization/127851 > > gcc/ChangeLog: > > * combine.cc (can_combine_p): Don't combine an insn that reads > volatile memory directly into an I3 whose source already reads > volatile memory. > > 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-1.c: New test. > * gcc.target/i386/volatile-fuse-order-2.c: New test. > * gcc.target/i386/volatile-fuse-order-3.c: New test.
OK for the x86 part. Thanks, Uros. > > Signed-off-by: Vineet Gupta <[email protected]> > --- > 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 | 7 ++++ > .../aarch64/volatile-fuse-order-1.c | 24 ++++++++++++++ > .../aarch64/volatile-fuse-order-2.c | 23 +++++++++++++ > .../gcc.target/i386/volatile-fuse-order-1.c | 25 +++++++++++++++ > .../gcc.target/i386/volatile-fuse-order-2.c | 32 +++++++++++++++++++ > .../gcc.target/i386/volatile-fuse-order-3.c | 31 ++++++++++++++++++ > 6 files changed, 142 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-1.c > create mode 100644 gcc/testsuite/gcc.target/i386/volatile-fuse-order-2.c > create mode 100644 gcc/testsuite/gcc.target/i386/volatile-fuse-order-3.c > > diff --git a/gcc/combine.cc b/gcc/combine.cc > index 429530816846..49288e950787 100644 > --- a/gcc/combine.cc > +++ b/gcc/combine.cc > @@ -2004,6 +2004,13 @@ 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 (a read-modify-write is ordered). */ > + rtx i3set = single_set (i3); > + if (succ == 0 > + && volatile_refs_p (i3set ? SET_SRC (i3set) : PATTERN (i3))) > + return false; > /* 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-1.c > b/gcc/testsuite/gcc.target/i386/volatile-fuse-order-1.c > new file mode 100644 > index 000000000000..70bacee284f8 > --- /dev/null > +++ b/gcc/testsuite/gcc.target/i386/volatile-fuse-order-1.c > @@ -0,0 +1,25 @@ > +/* { 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 put both reads into one compare insn, > + which gives them no order. */ > + > +/* > +**f: > +**... > +** movl x(|\(%rip\)), %eax > +** cmpl y(|\(%rip\)), %eax > +**... > +*/ > + > +extern volatile int x, y; > + > +int > +f (void) > +{ > + int a = x; > + int b = y; > + return a < b; > +} > diff --git a/gcc/testsuite/gcc.target/i386/volatile-fuse-order-2.c > b/gcc/testsuite/gcc.target/i386/volatile-fuse-order-2.c > new file mode 100644 > index 000000000000..8ea0bdaf3f46 > --- /dev/null > +++ b/gcc/testsuite/gcc.target/i386/volatile-fuse-order-2.c > @@ -0,0 +1,32 @@ > +/* { dg-do compile } */ > +/* { dg-options "-O2 -fomit-frame-pointer -masm=att" } */ > +/* { dg-final { check-function-bodies "ia32" "*ia32" "" { target { ia32 && { > ! *-*-darwin* } } } } } */ > +/* { dg-final { check-function-bodies "x64" "*x64" "" { target { { ! ia32 } > && { ! *-*-darwin* } } } } } */ > + > +/* Both reads of *P are in separate full expressions: the first read must > + be the one loaded into a register before the compare, which performs > + the second read. */ > + > +/* > +ia32f: > +ia32... > +ia32 movl \(%eax\), %edx > +ia32 cmpl \(%eax\), %edx > +ia32... > +*ia32 > + > +x64f: > +x64... > +x64 movl \(%(r|e)di\), %eax > +x64 cmpl \(%(r|e)di\), %eax > +x64... > +*x64 > +*/ > + > +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-3.c > b/gcc/testsuite/gcc.target/i386/volatile-fuse-order-3.c > new file mode 100644 > index 000000000000..5fe024338874 > --- /dev/null > +++ b/gcc/testsuite/gcc.target/i386/volatile-fuse-order-3.c > @@ -0,0 +1,31 @@ > +/* { dg-do compile } */ > +/* { dg-options "-O2 -fomit-frame-pointer -fno-pic -masm=att" } */ > +/* { dg-final { check-function-bodies "**" "" "" { target { ! *-*-darwin* } > } } } */ > + > +/* As volatile-fuse-order-1.c, but only the sign of the difference is > + used, so the two reads would end up as the operands of a MINUS inside > + the compare, (compare (minus (mem x) (mem y)) (const_int 0)), rather > + than directly as the compare's operands. x must still be read first. */ > + > +/* > +**f: > +**... > +** movl x(|\(%rip\)), %eax > +** cmpl y(|\(%rip\)), %eax > +**... > +*/ > + > +extern volatile int x, y; > +void g1 (void); > +void g2 (void); > + > +void > +f (void) > +{ > + int a = x; > + int b = y; > + if ((int) ((unsigned) a - (unsigned) b) < 0) > + g1 (); > + else > + g2 (); > +} > -- > 2.53.0-Meta >
