[PATCH] D56534: [Verifier] Add verification of unaligned atomic load/store

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

In D56534#1362347 <https://reviews.llvm.org/D56534#1362347>, @jfb wrote:

> In D56534#1362295 <https://reviews.llvm.org/D56534#1362295>, @jyknight wrote:
>
> > So, looking back at what prompted this...
> >
> >   a5b0e5585b1d r351295 - [InstCombine]Avoid introduction of unaligned mem access
> >
> > I think that fix is not the right answer...it's only papering over the issue of a generally incorrect optimization.
> >
> > That is, I think both of these previous commits:
> >
> >   f6651d4d9438 r332132 - [InstCombine] Handle atomic memset in the same way as regular memset
> >   8f30ec65b0a6 r332093 - [InstCombine] Unify handling of atomic memtransfer with non-atomic memtransfer
> >
> > are likely wrong and need to be reverted. The user asked for N atomic ops of size M, but LLVM is instead emitting 1 atomic op of size N * M.
>
>
> I'm not sure I agree: those are `unordered` memsets, which are basically @reames' memsets. I think it's up to him to give them semantics that are useful for his target.


Thank you.  I was about to say pretty much the same.  In particular, trying to claim that a transform is incorrect and needs reverted based on a proposed reading of a change to the LangRef which has not been posted for review, much less accepted seems to be going a definite step too far.


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

https://reviews.llvm.org/D56534