On 10/21/26 12:34 AM, Uros Bizjak wrote:
On Wed, Oct 21, 2026 at 9:06 AM Uros Bizjak <[email protected]> wrote:
On Wed, Oct 21, 2026 at 7:46 AM Richard Biener
<[email protected]> wrote:
On Wed, Oct 21, 2026 at 6:42 AM Jeff Law <[email protected]> wrote:
On 10/17/26 3:23 AM, Uros Bizjak wrote:
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?
But that means every target has to be sure to follow the rules. That
seems like a major development and maintenance headache.
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.
But it still has to produce semantically equivalent RTL which the
volatile case is not considering an RTL abstract machine?
True, because when *two or more* volatile memory operands get combined
into a single RTX pattern, we don't know the order of operands
anymore. x86 example is e.g. *add<dwi>3_doubleword_concat that swaps
and mixes two possibly volatile operands, with a counter example of
double mode add that has *one* possibly volatile volatile operand. If
middle-end ensures that only one volatile operand gets propagated into
RTX (please note that combine won't propagate memory over other
memory), then volatile accesses won't get reordered.
To illustrate the above, let's analyze how (unpatched) compiler
combines testcase from pr127851 (x and y are both volatiles):
5: r99:SI=[`x']
6: r100:SI=[`y']
7: flags:CCGC=cmp(r99:SI,r100:SI)
REG_DEAD r100:SI
REG_DEAD r99:SI
combine pass:
Trying 5 -> 7:
5: r99:SI=[`x']
7: flags:CCGC=cmp(r99:SI,r100:SI)
REG_DEAD r100:SI
REG_DEAD r99:SI
Can't combine i2 into i3 <- i2 passes insn 6, a volatile read, so
already rejected by combine.
Trying 6 -> 7:
6: r100:SI=[`y']
7: flags:CCGC=cmp(r99:SI,r100:SI)
REG_DEAD r100:SI
REG_DEAD r99:SI
Successfully matched this instruction:
(set (reg:CCGC 17 flags)
(compare:CCGC (reg/v:SI 99 [ a ])
(mem/v/c:SI (symbol_ref:DI ("y") [flags 0x40] <var_decl
0x7fd1a93d6ed8 y>) [1 y+0 S4 A32])))
allowing combination of insns 6 and 7 <- OK
Trying 5 -> 7:
5: r99:SI=[`x']
7: flags:CCGC=cmp(r99:SI,[`y'])
REG_DEAD r99:SI
Successfully matched this instruction:
(set (reg:CCGC 17 flags)
(compare:CCGC (mem/v/c:SI (symbol_ref:DI ("x") [flags 0x40]
<var_decl 0x7fd1a93d6e40 x>) [1 x+0 S4 A32])
(mem/v/c:SI (symbol_ref:DI ("y") [flags 0x40] <var_decl
0x7fd1a93d6ed8 y>) [1 y+0 S4 A32])))
allowing combination of insns 5 and 7 <- this should *NOT* be
allowed, there is already volatile read in the RTX, so combination
with another volatile read should be rejected.
IMO rejecting the last combination in the middle-end would resolve
this class of problems.
Sorry for not chiming in earlier, duty called for being elsewhere :-)
So it seems the consensus is that this is not just the backend's duty to
fix this as it still leaves holes. If combine is still the right place
to do this (vs. the middle-end) I can resume there, addressing some of
the things Jeff wanted in v3. The x86 cmp fix landing makes the testing
a little harder now but the tips about concat patterns help.
Thx,
-Vineet
