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

[Momchil Velikov via Phabricator via llvm-commits]

chill added a comment.

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.




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

https://reviews.llvm.org/D64414