On Fri, Oct 23, 2026 at 7:00 AM Uros Bizjak <[email protected]> wrote: > > 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.
Eh, no. Combine can split insns internally, and we have to reject combined insns with multiple volatile ops before they get split. Uros.
