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

[JF Bastien via Phabricator via llvm-commits]

jfb added a comment.

In D56534#1362295 <https://reviews.llvm.org/D56534#1362295>, @jyknight wrote:

> In D56534#1362174 <https://reviews.llvm.org/D56534#1362174>, @jfb wrote:
>
> > In D56534#1362171 <https://reviews.llvm.org/D56534#1362171>, @jyknight wrote:
> >
> > > I believe that instead of this patch, the LangRef should be updated to say:
> > >
> > > 1. Misaligned atomics in the IR are allowed, and work correctly (but will most likely be inefficient).
> >
> >
> > This sounds fine. However, the premise of this patch IIUC was that LLVM might add unaligned atomics. Is this something we should worry about? If so, my original comment stands: the verifier should make sure the only unaligned atomic accesses came directly at the user's request.
>
>
> Hrm -- that premise now worries me, because it implies that we have optimization passes which are merging atomic operations. I think that's incorrect, *even if* such merging results in aligned accesses.
>
> 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.


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

https://reviews.llvm.org/D56534