On Fri, Oct 23, 2026 at 1:23 AM Vineet Gupta <[email protected]> wrote: > > On 10/21/26 2:17 PM, 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, so whatever later reloads or splits > > it picks the order of the two volatile reads, and the sequence point > > between them is lost. can_combine_p checks insns between INSN and I3 > > for volatile references, but not I3 itself. > > > > The x86 compare patterns now reject such operands (dc57b45e7a79), but > > other patterns accept them too: for "h = hi; l = lo;" combined into > > ((dw) h << N) | l, the x86 doubleword concat patterns take both halves > > as memory operands and their split reads lo first. Rather than fixing > > each pattern, refuse in combine to combine an insn that reads volatile > > memory directly into an I3 that already reads volatile memory. > > > > The new volatile_reads_p walks I3's pattern: every SET in a PARALLEL > > counts, and so does anything read to form a stored location (a MEM's > > address, a ZERO_EXTRACT's position and size), but a volatile MEM that is > > only stored to does not, since an insn does all its reads before its > > writes. That does not hold for an asm, whose template may access its > > operands in any order, so for an asm I3 any volatile reference refuses. > > > > 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. > > (can_combine_p): Use it to refuse combining an insn that reads > > volatile memory directly into an I3 that already reads volatile > > memory; for an asm I3, refuse on any volatile reference. > > > > 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. > > > > Signed-off-by: Vineet Gupta <[email protected]> > > --- > > 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 | 58 +++++++++++++++++++ > > .../aarch64/volatile-fuse-order-1.c | 24 ++++++++ > > .../aarch64/volatile-fuse-order-2.c | 23 ++++++++ > > .../gcc.target/i386/volatile-fuse-order-4.c | 49 ++++++++++++++++ > > .../gcc.target/i386/volatile-fuse-order-5.c | 45 ++++++++++++++ > > .../gcc.target/i386/volatile-fuse-order-6.c | 35 +++++++++++ > > 6 files changed, 234 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 > > > > diff --git a/gcc/combine.cc b/gcc/combine.cc > > index c9daefc905c3..b369914e49e4 100644 > > --- a/gcc/combine.cc > > +++ b/gcc/combine.cc > > @@ -1723,6 +1723,53 @@ set_nonzero_bits_and_sign_copies (rtx x, const_rtx > > set, void *data) > > } > > } > > > > +/* Return true if insn pattern X reads volatile memory. */ > > + > > +static bool > > +volatile_reads_p (const_rtx x) > > +{ > > + switch (GET_CODE (x)) > > + { > > + case PARALLEL: > > + /* Every element counts, not just the one single_set would pick. */ > > + for (int i = 0; i < XVECLEN (x, 0); i++) > > + if (volatile_reads_p (XVECEXP (x, 0, i))) > > + return true; > > + return false; > > + > > + case SET: > > + { > > + /* Strip down to the stored location. The position and size of a > > + bit-field are read. */ > > + rtx dest = SET_DEST (x); > > + while (GET_CODE (dest) == SUBREG > > + || GET_CODE (dest) == STRICT_LOW_PART > > + || GET_CODE (dest) == ZERO_EXTRACT) > > + { > > + if (GET_CODE (dest) == ZERO_EXTRACT > > + && (volatile_refs_p (XEXP (dest, 1)) > > + || volatile_refs_p (XEXP (dest, 2)))) > > + return true; > > + dest = XEXP (dest, 0); > > + } > > + /* A stored MEM is not read, and since an insn does all its reads > > + before its writes, a volatile store cannot move ahead of a > > + volatile read in the same insn. Its address is read, though, > > + e.g. (mem (mem/v ...)). */ > > + if (MEM_P (dest) && volatile_refs_p (XEXP (dest, 0))) > > + return true; > > + return volatile_refs_p (SET_SRC (x)); > > + } > > + > > + case CLOBBER: > > + /* A clobbered MEM is not read, but its address is. */ > > + return MEM_P (XEXP (x, 0)) && volatile_refs_p (XEXP (XEXP (x, 0), > > 0)); > > + > > + default: > > + return volatile_refs_p (x); > > + } > > +} > > + > > /* See if INSN can be combined into I3. PRED, PRED2, SUCC and SUCC2 are > > optionally insns that were previously combined into I3 or that will be > > combined into the merger of INSN and I3. The order is PRED, PRED2, > > @@ -2004,6 +2051,17 @@ 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, since all of an insn's reads happen > > + before its writes, but not in an asm, whose template may access its > > + operands in any order. (When INSN goes through SUCC, combine may > > + split the result back into two insns that keep the reads ordered.) > > */ > > + if (succ == 0 > > + && (asm_noperands (PATTERN (i3)) >= 0 > > + ? volatile_refs_p (PATTERN (i3)) > > + : volatile_reads_p (PATTERN (i3)))) > > + return false; > > I don't want to jump the gun on the due review process but seems like > there is some feedback from gcc sashiko which Uroz shared with me [1] > and the issue it found seems legit. > > [1] > https://gcc.sashiko.dev/#/patchset/20261021211758.3144964-1-vineet.gupta%40linux.dev > > Essentially the "succ == 0" check above implies this only happens for I2 > -> I3 combine, but leaves the 3 (or 4) combine cases broken. e.g. Test > below on x86, z is incorrectly read before y (with my v4 and despite > Uroz's i386.md fix) > > extern volatile int x, y, z; > int noarg (void) { int t1 = x; int t2 = y; int t3 = z; return t1 + > t2 + t3; } > > Dropping the check fixes this test above but regresses other valid > cases: the test from original introduction of > -ffuse-ops-with-volatile-access.
FYI, the i386.md fix was specific to compare patterns, and as Jeff pointed out, that approach results in a maintenance hell, as it requires manually changing all RTX patterns that accept two or more memory operands with the insn check that allows at most one volatile operand. AFAICS, the same effect can be achieved in recog_for_combine with some RTX walker that rejects invalid RTXes at the point of recognition. I don't know if this is the most optimal solution, but as shown with my i386.md experiment, it works. Uros.
