[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#1576859 <https://reviews.llvm.org/D64414#1576859>, @chill wrote:

> In D64414#1576588 <https://reviews.llvm.org/D64414#1576588>, @jdoerfert wrote:
>
> > In D64414#1576554 <https://reviews.llvm.org/D64414#1576554>, @chill wrote:
> >
> > > In D64414#1576252 <https://reviews.llvm.org/D64414#1576252>, @jdoerfert wrote:
> > >
> > > > I'm a bit lost. What intrinsic do we have that is "no-mem" but has side-effects?
> > >
> > >
> > > E.g  `__builtin_arm_tcommit`, which I'm adding in this series, or  `__builtin_arm_irg` (which is *should* be `NoMem`, but is not, perhaps as a workaround for this issue).
> >
> >
> > (Sorry if I'm way off here but I'd like to understand this better)
> >
> > `tcommit` makes the transaction state visible, correct? If so, I'd argue it modifies the memory (especially for other threads). Maybe my confusion stems from the IR vs MIR conflict here but in IR, I would not like `tcommit` to be `readnone` for multiple reasons, including memory movement around it.
>
>
> `tcommit` does not modify memory (at least in my understanding what "modify memory" means), in the sense that if I do:
>
>   *p = x;
>   tcommit();
>   z = *p
>
>
> I would expect `x == z`, i.e the above to be equivalent to
>
>   *p = x;
>   tcommit();
>   z = x;
>
>
> Definitely we wouldn't like memory operations to be moved around transactional primitives.


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.


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

https://reviews.llvm.org/D64414