On Tue, Oct 28, 2025 at 9:31 AM Richard Biener <[email protected]> wrote: > > On Tue, Oct 28, 2025 at 9:29 AM Richard Biener > <[email protected]> wrote: > > > > On Tue, Oct 28, 2025 at 7:03 AM H.J. Lu <[email protected]> wrote: > > > > > > On Tue, Oct 28, 2025 at 1:26 PM Uros Bizjak <[email protected]> wrote: > > > > > > > > On Mon, Oct 27, 2025 at 11:56 PM H.J. Lu <[email protected]> wrote: > > > > > > > > > > > > IMO the -fcombine-op-with-volatile-memory-load is quite a bad > > > > > > > > name. > > > > > > > > Options should have names that mean something to users. As > > > > > > > > this seems to > > > > > > > > supposed to be a target opt-in I'd suggest to not expose this > > > > > > > > to users > > > > > > > > (or if targets are concerned, via some -m...) but instead make > > > > > > > > this a > > > > > > > > > > > > > > -mvolatile-memory-load? > > > > > > > > > > > > -mfuse-ops-with-volatile-access? > > > > > > > > > > > > So as to eventually also allow RMW? > > > > > > > > > > Yes. We can add the LOCK prefix for RMW if it is allowed. But will > > > > > it > > > > > improve performance? The LOCK prefix may be expensive. > > > > > > > > No, you don't need the LOCK prefix for volatile RMW access. LOCK is > > > > needed for inter-thread synchronization, which is orthogonal to RMW > > > > access. > > > > > > > > What Richard mentions is something like PR3506 [1], a step further > > > > from propagating memory input, where also memory output is propagated > > > > to form the instruction with RMW access. As an example, maybe you want > > > > to increase the value stored at some memory-mapped IO. The location > > > > has to be marked volatile, otherwise optimizers can figure out that > > > > the memory location is unused and simply remove the update of "unused" > > > > location. > > > > > > > > So, in the PR it is argued that: > > > > > > > > movl y, %eax > > > > incl %eax > > > > movl %eax, y > > > > > > > > access to volatile location is the same as: > > > > > > > > incl y > > > > > > > > and that the latter does not violate volatile correctness. > > > > > > > > [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=3506 > > > > > > > > Uros. > > > > > > This can be a followup patch. > > > > it should be, I was merely mentioning this to make sure the command-line > > option, > > if we add one, would be one that would cover this case as well (so not > > use 'load' > > in the name). > > Oh, and I'd expect eventual fallout for define-insn-and-split or > UNSPECs that fail > to check volatileness on MEM operands but ends up duplicating or splitting > them > or merging them. Basically targets that rely on volatile MEMs not appearing > as > operands to RTL ops.
Please also see attachment 55167 in PR60749 [1]. [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=60749#c2 Uros.
