On Fri, Oct 16, 2026 at 6:03 PM Jeff Law <[email protected]> wrote: > > > > On 10/14/26 3:15 AM, 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 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. > > > > 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. */ > > } > So you're going to hate me :-) > > RTL is largely unstructured. So you *could* have something like (mem > (mem ... )) as the destination. The inner mem is a read that supplies > the address to the outer mem and that inner mem might be volatile. vax > might have this capability, though I don't know for sure. > > This would also seem to miss the case when the insn is not a single set, > so maybe it's a PARALLEL with a bunch of reads/writes inside or other > forms that do not satisfy single_set. > > So while I think your patch improves things, I think it's still leaving > some pretty big holes.
How about we leave combine as is and leave the task of checking patterns w.r.t volatile operands to the target code? Combine is just a fancy forward propagator, it creates more or less sensible combined patterns (and performs some RTX simplifications) and leaves it to the target-dependent code to check if the combined pattern is acceptable. Following this approach, the target should *also* check if the combined pattern is acceptable when volatile operands are combined. x86 code is apparently almost clean regarding volatile operands handling (otherwise linux would break left and right), but there are some corner cases remaining, and this patch tries to handle one of them in a generic way. Since targets can handle volatiles in different ways, a generic approach is IMO not the correct way. To solve the presented problem for x86, we should prevent combinations where any of two memory operands is volatile. So, just add: !(MEM_P (operands[0]) && MEM_P (operands[1]) && (MEM_VOLATILE_P (operands[0]) || MEM_VOLATILE_P (operands[1]))) to the insn constraint of *cmp<mode>_1 and *cmp<mode>_minus_1 insn patterns (and apparently also @ccmp<mode>). This will prevent unwanted combinations of memory operands where one of them is volatile, while still allowing all other non-problematic combinations, including two non-volatile mems. Uros.
