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

[Guillaume Chatelet via Phabricator via llvm-commits]

gchatelet marked an inline comment as done.
gchatelet added inline comments.


================
Comment at: llvm/include/llvm/IR/IRBuilder.h:1746
+    return Insert(new AtomicCmpXchgInst(
+        Ptr, Cmp, New, Alignment, SuccessOrdering, FailureOrdering, SSID));
   }
----------------
jfb wrote:
> 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.
With this patch, the alignment is still not user accessible, just defined a bit higher in the call hierarchy so I think it's fine. This patch is NFC.
Next patch will make this change visible to users so we'll have to document it in LangRef, @jfb I'll loop you in so you can proofread it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83136