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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

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.


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

https://reviews.llvm.org/D64414