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

[James Y Knight via Phabricator via llvm-commits]

jyknight added a comment.

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.


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

https://reviews.llvm.org/D56534