On 10/16/26 21:31, Jeff Law 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 :-)
You can never make me hate you. So bring it on !
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.
Sorry it is not clear what the guidance is, we let combine do what it
does today and address is else where. Or would rather me fix the combine
part differently and/or handle PARALLEL or other things.
So while I think your patch improves things, I think it's still leaving
some pretty big holes.
The question is: Is this fix punching a bigger hole than what it is
today or is it patching it partially. I don't want to rush this change.
But if it is an improvement on today, can we land this first and address
the rest as a follow-on. And I promise I won't run away as I still need
you to review my future patches ;-)
Thx,
-Vineet
