[PATCH] D64414: Do not set ReadNone attribute on intrinsics with side effects

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a subscriber: arsenm.
jdoerfert added a comment.

In D64414#1580538 <https://reviews.llvm.org/D64414#1580538>, @chill wrote:

> In D64414#1578661 <https://reviews.llvm.org/D64414#1578661>, @jdoerfert wrote:
>
> > In D64414#1577844 <https://reviews.llvm.org/D64414#1577844>, @chill wrote:
> >
> > > In D64414#1576955 <https://reviews.llvm.org/D64414#1576955>, @jdoerfert wrote:
> > >
> > > > What if a different thread causes an abort before the commit? store-load forwarding would then cause an inconsistent view, wouldn't it?
> > > >  I find this is like atomic loads that also "write" memory if the synchronization is strong enough to make other modifications visible at that point.
> > >
> > >
> > > If the transaction is aborted before or by the `tcommit`, it'll be automatically rolled back to the `tstart`, which initiated it, discarding
> > >  any register and memory writes, and won't reach the instruction after the `tcommit`.
> >
> >
> > I agree, that is what needs to happen. Maybe I misunderstood what your example was supposed to show. The way I did understand it, I figured you want the store to load propagation to happen across the `tcommit` instruction. That has to be forbidden in the IR as it will otherwise happen and break the atomicity of the transaction. To forbid it one would give the `tcommit` intrinsic a memory effect. Now, I think it actually has a memory effect: Similar to sequential consistent atomic loads, `tcommit` synchronizes, thereby making memory changes visible to the thread.
> >
> > We can move the discussion to the main patch if you want.
>
>
> Sorry, I wasn't clear, my example was just to clarify what I mean by "does not  modify memory".
>
> So, back the original motivation for the patch, let's look at  `__builtin_arm_ttest` and  `__builtin_arm_irg`  (which map 1:1 to an LLVM intrnisic and to an AArch64 instruction, so I use a short name to refer to all three)
>
> - `ttest` is set to have side affects, so it's not reordered with `tstart` and `tcommit`;  it's OK to move memory reads/writes around it, so it's also `IntrNoMem`; in that state tablegen would set `readnone` attribute. Bu then for `readnone`:
>
> > On a function, this attribute indicates that the function computes its result (or decides to unwind an exception) based strictly on its arguments, without dereferencing any pointer arguments or otherwise accessing any mutable state (e.g. memory, control registers, etc)
>
> and `ttest` *does* access mutable state. TBH, if the result of `ttest` is dead, there's no problem discarding the call, it just contradicts with the LLVM spec.
>
> - It's also fine to reorder memory read/writes around `irg`; but it has actual side-effects, so it's not OK to discard it even if its result is read; if we define `irg` with `IntrNoMem` and `IntrHasSideEffect` it'll get `readnone` and might be discarded.


I'm not fine with the "contradicting LLVM spec" part.

Though, you and @arsenm convinced me that we, for now, need different control for intrinisic in the backend. That is, you want `readnone` set for them in t least right nowthe backend but not `readnone` set for them in the IR, correct? If that is so, this patch is fine with me (LGTM).

(For the record: I did initially not understand why one would like to have both, `readnone` and `side-effects` for an intrinsics. I still think that is wrong from the middle-end (IR) point of view, e.g., tcommit in IR has to carry a "write memory" effect, but it seems to be common in the backend.)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64414/new/

https://reviews.llvm.org/D64414