[PATCH] D83136: [NFC] Adding the align attribute on Atomic{CmpXchg|RMW}Inst

[JF Bastien via Phabricator via llvm-commits]

jfb added inline comments.


================
Comment at: llvm/include/llvm/IR/IRBuilder.h:1746
+    return Insert(new AtomicCmpXchgInst(
+        Ptr, Cmp, New, Alignment, SuccessOrdering, FailureOrdering, SSID));
   }
----------------
jyknight wrote:
> jfb wrote:
> > Are types always rounded to a power of two at this point?
> > 
> > i.e. what does this do: `struct { char c[3]; };` ?
> > 
> > Also, I think this is wrong for non-lock-free types. The alignment requirement is lower on those.
> This is just encoding the pre-existing behavior -- you cannot currently create an cmpxchg instruction with alignment other than the size of the type.
> 
> Right now, you _also_ cannot create a cmpxchg instruction with other than integral or pointer types, which -- in any _current_ llvm backend, afaik -- have non-power-of-2 sizes.
> 
> Upcoming changes will plumb through a required alignment argument everywhere, and then we'll be rid of this weird hardcoded special alignment behavior here.
That sounds good to me. FWIW I checked and we get the following today:
```
  %3 = bitcast %"struct.std::__1::atomic"* %0 to i32*
  %4 = zext i24 %1 to i32
  %5 = cmpxchg i32* %3, i32 %4, i32 %4 seq_cst seq_cst
  ret void
```
That being said, if we're going to allow other things to come into a cmpxchg in the future (i.e. remove the need to bitcast) then I want to make sure we encode the right requirements here, right now. I agree that they're enforced later in the code when the instruction is created, but that won't always be the case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83136