On Wed, Oct 21, 2026 at 8:15 PM Vineet Gupta <[email protected]> wrote: > > 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.
Just revert my patch locally, it is effectively only a couple of lines. My patch should be reverted anyway when the combine is improved to handle volatiles correctly. Thanks, Uros.
