[Alignment][NFC] Migrate AtomicExpandPass to Align

[gchatelet]

Summary

This is a followup on D78403.
I'm unsure about getAtomicOpAlign overloads that take AtomicRMWInst and AtomicCmpXchgInst, shouldn't getAlign provide the correct answer already?

[efriedma]

I'm unsure about getAtomicOpAlign overloads that take AtomicRMWInst and AtomicCmpXchgInst, shouldn't getAlign provide the correct answer already?

getAlign() on what? AtomicRMWInst and AtomicCmpXchgInst don't have a getAlign() method. (It might make sense to add.)

llvm/lib/CodeGen/AtomicExpandPass.cpp

1516–1517

Can just use I->getAlign() here.

[gchatelet]

• Add getAlign to AtomicCmpXchgInst/AtomicRMWInst and simplify the code

[gchatelet]

@jyknight does that look like a step in the right direction regarding https://bugs.llvm.org/show_bug.cgi?id=27168?

[gchatelet]

In D81369#2080923, @efriedma wrote:

I'm unsure about getAtomicOpAlign overloads that take AtomicRMWInst and AtomicCmpXchgInst, shouldn't getAlign provide the correct answer already?

getAlign() on what? AtomicRMWInst and AtomicCmpXchgInst don't have a getAlign() method. (It might make sense to add.)

My bad, I should have read PR27168 more carefully before requesting comments. I've added the getAlign() methods and looped @jyknight in.

[jfb]

Would you update anything in the langref w.r.t. these two instructions? The RMW langref doesn't say anything, and the cmpxchg part says:

The pointer passed into cmpxchg must have alignment greater than or equal to the size in memory of the operand.

You're preserving the natural alignment requirement in the code, but langref seems slightly imprecise.

[gchatelet]

rebase

[efriedma]

LGTM. We probably should update langref, but I don't think we need to block this patch.

llvm/include/llvm/IR/Instructions.h

528

Add a doc comment here?

[gchatelet]

• Fix documentation

[gchatelet]

@jfb @efriedma @jyknight I intend to wait for D81662 to introduce a proper alignment and fix https://bugs.llvm.org/show_bug.cgi?id=27168
Then we can clarify LangRef.

[jfb]

Adding aliment to these operations will also require updating MergeFuncs /cc @vish99 @hiraditya

[gchatelet]

In D81369#2120501, @jfb wrote:

Adding aliment to these operations will also require updating MergeFuncs /cc @vish99 @hiraditya

SGTM, I'll make sure I'll loop them in.
I'll go ahead and submit this one in the meantime.
Thank you @jfb

[hiraditya]

@vish99 will your work in https://reviews.llvm.org/D82892
check alignment changes as well?

[vish99]

@vish99 will your work in https://reviews.llvm.org/D82892 check alignment changes as well?

Currently no, since both these instructions don't have an explicit alignment attribute. It can be easily added if the attribute exists.

[jyknight]

Thanks for the cleanup!

Belatedly LGTM, and I look forward to your fix to add alignment to atomicrmw/cmpxchg in the future. :)

llvm/include/llvm/IR/Instructions.h

AtomicCmpXchgInst(Value *Ptr, Value *Cmp, Value *NewVal,
                     AtomicOrdering FailureOrdering,
                     SyncScope::ID SSID, BasicBlock *InsertAtEnd);
 
   // allocate space for exactly three operands
   void *operator new(size_t s) {
     return User::operator new(s, 3);
   }
 
+  /// Always returns the natural type alignment.
+  /// FIXME: Introduce a proper alignment
+  /// https://bugs.llvm.org/show_bug.cgi?id=27168
+  Align getAlign() const;
+
   /// Return true if this is a cmpxchg from a volatile memory
   /// location.
   ///
   bool isVolatile() const {
     return getSubclassDataFromInstruction() & 1;
   }
 
   /// Specify whether this is a volatile cmpxchg.
public:
   }
 
   void setOperation(BinOp Operation) {
     unsigned short SubclassData = getSubclassDataFromInstruction();
     setInstructionSubclassData((SubclassData & 31) |
                                (Operation << 5));
   }
 
+  /// Always returns the natural type alignment.
+  /// FIXME: Introduce a proper alignment
+  /// https://bugs.llvm.org/show_bug.cgi?id=27168
+  Align getAlign() const;
+
   /// Return true if this is a RMW on a volatile memory location.
   ///
   bool isVolatile() const {
     return getSubclassDataFromInstruction() & 1;
   }
 
   /// Specify whether this is a volatile RMW or not.
   ///

llvm/lib/CodeGen/AtomicExpandPass.cpp

static Value *insertRMWCmpXchgLoop(
         function_ref<Value *(IRBuilder<> &, Value *)> PerformOp,
         CreateCmpXchgInstFun CreateCmpXchg);
     bool tryExpandAtomicCmpXchg(AtomicCmpXchgInst *CI);
 
     bool expandAtomicCmpXchg(AtomicCmpXchgInst *CI);
     bool isIdempotentRMW(AtomicRMWInst *RMWI);
     bool simplifyIdempotentRMW(AtomicRMWInst *RMWI);
 
-    bool expandAtomicOpToLibcall(Instruction *I, unsigned Size, unsigned Align,
+    bool expandAtomicOpToLibcall(Instruction *I, unsigned Size, Align Alignment,
                                  Value *PointerOperand, Value *ValueOperand,
                                  Value *CASExpected, AtomicOrdering Ordering,
                                  AtomicOrdering Ordering2,
                                  ArrayRef<RTLIB::Libcall> Libcalls);
     void expandAtomicLoadToLibcall(LoadInst *LI);
     void expandAtomicStoreToLibcall(StoreInst *LI);
     void expandAtomicRMWToLibcall(AtomicRMWInst *I);
     void expandAtomicCASToLibcall(AtomicCmpXchgInst *I);
static unsigned getAtomicOpSize(AtomicRMWInst *RMWI) {
   return DL.getTypeStoreSize(RMWI->getValOperand()->getType());
 }
 
 static unsigned getAtomicOpSize(AtomicCmpXchgInst *CASI) {
   const DataLayout &DL = CASI->getModule()->getDataLayout();
   return DL.getTypeStoreSize(CASI->getCompareOperand()->getType());
 }
 
-// Helper functions to retrieve the alignment of atomic instructions.
-static unsigned getAtomicOpAlign(LoadInst *LI) {
-  unsigned Align = LI->getAlignment();
-  // In the future, if this IR restriction is relaxed, we should
-  // return DataLayout::getABITypeAlignment when there's no align
-  // value.
-  assert(Align != 0 && "An atomic LoadInst always has an explicit alignment");
-  return Align;
-}
-
-static unsigned getAtomicOpAlign(StoreInst *SI) {
-  unsigned Align = SI->getAlignment();
-  // In the future, if this IR restriction is relaxed, we should
-  // return DataLayout::getABITypeAlignment when there's no align
-  // value.
-  assert(Align != 0 && "An atomic StoreInst always has an explicit alignment");
-  return Align;
-}
-
-static unsigned getAtomicOpAlign(AtomicRMWInst *RMWI) {
-  // TODO(PR27168): This instruction has no alignment attribute, but unlike the
-  // default alignment for load/store, the default here is to assume
-  // it has NATURAL alignment, not DataLayout-specified alignment.
-  const DataLayout &DL = RMWI->getModule()->getDataLayout();
-  return DL.getTypeStoreSize(RMWI->getValOperand()->getType());
-}
-
-static unsigned getAtomicOpAlign(AtomicCmpXchgInst *CASI) {
-  // TODO(PR27168): same comment as above.
-  const DataLayout &DL = CASI->getModule()->getDataLayout();
-  return DL.getTypeStoreSize(CASI->getCompareOperand()->getType());
-}
-
 // Determine if a particular atomic operation has a supported size,
 // and is of appropriate alignment, to be passed through for target
 // lowering. (Versus turning into a __atomic libcall)
 template <typename Inst>
 static bool atomicSizeSupported(const TargetLowering *TLI, Inst *I) {
   unsigned Size = getAtomicOpSize(I);
-  unsigned Align = getAtomicOpAlign(I);
-  return Align >= Size && Size <= TLI->getMaxAtomicSizeInBitsSupported() / 8;
+  Align Alignment = I->getAlign();
+  return Alignment >= Size &&
+         Size <= TLI->getMaxAtomicSizeInBitsSupported() / 8;
 }
 
 bool AtomicExpand::runOnFunction(Function &F) {
   auto *TPC = getAnalysisIfAvailable<TargetPassConfig>();
   if (!TPC)
     return false;
 
   auto &TM = TPC->getTM<TargetMachine>();
bool llvm::expandAtomicRMWToCmpXchg(AtomicRMWInst *AI,
   return true;
 }
 
 // In order to use one of the sized library calls such as
 // __atomic_fetch_add_4, the alignment must be sufficient, the size
 // must be one of the potentially-specialized sizes, and the value
 // type must actually exist in C on the target (otherwise, the
 // function wouldn't actually be defined.)
-static bool canUseSizedAtomicCall(unsigned Size, unsigned Align,
+static bool canUseSizedAtomicCall(unsigned Size, Align Alignment,
                                   const DataLayout &DL) {
   // TODO: "LargestSize" is an approximation for "largest type that
   // you can express in C". It seems to be the case that int128 is
   // supported on all 64-bit platforms, otherwise only up to 64-bit
   // integers are supported. If we get this wrong, then we'll try to
   // call a sized libcall that doesn't actually exist. There should
   // really be some more reliable way in LLVM of determining integer
   // sizes which are valid in the target's C ABI...
   unsigned LargestSize = DL.getLargestLegalIntTypeSizeInBits() >= 64 ? 16 : 8;
-  return Align >= Size &&
+  return Alignment >= Size &&
          (Size == 1 || Size == 2 || Size == 4 || Size == 8 || Size == 16) &&
          Size <= LargestSize;
 }
 
 void AtomicExpand::expandAtomicLoadToLibcall(LoadInst *I) {
   static const RTLIB::Libcall Libcalls[6] = {
       RTLIB::ATOMIC_LOAD,   RTLIB::ATOMIC_LOAD_1, RTLIB::ATOMIC_LOAD_2,
       RTLIB::ATOMIC_LOAD_4, RTLIB::ATOMIC_LOAD_8, RTLIB::ATOMIC_LOAD_16};
   unsigned Size = getAtomicOpSize(I);
-  unsigned Align = getAtomicOpAlign(I);
 
   bool expanded = expandAtomicOpToLibcall(
-      I, Size, Align, I->getPointerOperand(), nullptr, nullptr,
+      I, Size, I->getAlign(), I->getPointerOperand(), nullptr, nullptr,
       I->getOrdering(), AtomicOrdering::NotAtomic, Libcalls);
   (void)expanded;
   assert(expanded && "expandAtomicOpToLibcall shouldn't fail tor Load");
 }
 
 void AtomicExpand::expandAtomicStoreToLibcall(StoreInst *I) {
   static const RTLIB::Libcall Libcalls[6] = {
       RTLIB::ATOMIC_STORE,   RTLIB::ATOMIC_STORE_1, RTLIB::ATOMIC_STORE_2,
       RTLIB::ATOMIC_STORE_4, RTLIB::ATOMIC_STORE_8, RTLIB::ATOMIC_STORE_16};
   unsigned Size = getAtomicOpSize(I);
-  unsigned Align = getAtomicOpAlign(I);
 
   bool expanded = expandAtomicOpToLibcall(
-      I, Size, Align, I->getPointerOperand(), I->getValueOperand(), nullptr,
-      I->getOrdering(), AtomicOrdering::NotAtomic, Libcalls);
+      I, Size, I->getAlign(), I->getPointerOperand(), I->getValueOperand(),
+      nullptr, I->getOrdering(), AtomicOrdering::NotAtomic, Libcalls);
   (void)expanded;
   assert(expanded && "expandAtomicOpToLibcall shouldn't fail tor Store");
 }
 
 void AtomicExpand::expandAtomicCASToLibcall(AtomicCmpXchgInst *I) {
   static const RTLIB::Libcall Libcalls[6] = {
       RTLIB::ATOMIC_COMPARE_EXCHANGE,   RTLIB::ATOMIC_COMPARE_EXCHANGE_1,
       RTLIB::ATOMIC_COMPARE_EXCHANGE_2, RTLIB::ATOMIC_COMPARE_EXCHANGE_4,
       RTLIB::ATOMIC_COMPARE_EXCHANGE_8, RTLIB::ATOMIC_COMPARE_EXCHANGE_16};
   unsigned Size = getAtomicOpSize(I);
-  unsigned Align = getAtomicOpAlign(I);
 
   bool expanded = expandAtomicOpToLibcall(
-      I, Size, Align, I->getPointerOperand(), I->getNewValOperand(),
+      I, Size, I->getAlign(), I->getPointerOperand(), I->getNewValOperand(),
       I->getCompareOperand(), I->getSuccessOrdering(), I->getFailureOrdering(),
       Libcalls);
   (void)expanded;
   assert(expanded && "expandAtomicOpToLibcall shouldn't fail tor CAS");
 }
 
 static ArrayRef<RTLIB::Libcall> GetRMWLibcall(AtomicRMWInst::BinOp Op) {
   static const RTLIB::Libcall LibcallsXchg[6] = {
static ArrayRef<RTLIB::Libcall> GetRMWLibcall(AtomicRMWInst::BinOp Op) {
   }
   llvm_unreachable("Unexpected AtomicRMW operation.");
 }
 
 void AtomicExpand::expandAtomicRMWToLibcall(AtomicRMWInst *I) {
   ArrayRef<RTLIB::Libcall> Libcalls = GetRMWLibcall(I->getOperation());
 
   unsigned Size = getAtomicOpSize(I);
-  unsigned Align = getAtomicOpAlign(I);
 
   bool Success = false;
   if (!Libcalls.empty())
     Success = expandAtomicOpToLibcall(
-        I, Size, Align, I->getPointerOperand(), I->getValOperand(), nullptr,
-        I->getOrdering(), AtomicOrdering::NotAtomic, Libcalls);
+        I, Size, I->getAlign(), I->getPointerOperand(), I->getValOperand(),
+        nullptr, I->getOrdering(), AtomicOrdering::NotAtomic, Libcalls);
 
   // The expansion failed: either there were no libcalls at all for
   // the operation (min/max), or there were only size-specialized
   // libcalls (add/sub/etc) and we needed a generic. So, expand to a
   // CAS libcall, via a CAS loop, instead.
   if (!Success) {
     expandAtomicRMWToCmpXchg(I, [this](IRBuilder<> &Builder, Value *Addr,
                                        Value *Loaded, Value *NewVal,

 
 // A helper routine for the above expandAtomic*ToLibcall functions.
 //
 // 'Libcalls' contains an array of enum values for the particular
 // ATOMIC libcalls to be emitted. All of the other arguments besides
 // 'I' are extracted from the Instruction subclass by the
 // caller. Depending on the particular call, some will be null.
 bool AtomicExpand::expandAtomicOpToLibcall(
-    Instruction *I, unsigned Size, unsigned Align, Value *PointerOperand,
+    Instruction *I, unsigned Size, Align Alignment, Value *PointerOperand,
     Value *ValueOperand, Value *CASExpected, AtomicOrdering Ordering,
     AtomicOrdering Ordering2, ArrayRef<RTLIB::Libcall> Libcalls) {
   assert(Libcalls.size() == 6);
 
   LLVMContext &Ctx = I->getContext();
   Module *M = I->getModule();
   const DataLayout &DL = M->getDataLayout();
   IRBuilder<> Builder(I);
   IRBuilder<> AllocaBuilder(&I->getFunction()->getEntryBlock().front());
 
-  bool UseSizedLibcall = canUseSizedAtomicCall(Size, Align, DL);
+  bool UseSizedLibcall = canUseSizedAtomicCall(Size, Alignment, DL);
   Type *SizedIntTy = Type::getIntNTy(Ctx, Size * 8);
 
-  const llvm::Align AllocaAlignment(DL.getPrefTypeAlignment(SizedIntTy));
+  const Align AllocaAlignment = DL.getPrefTypeAlign(SizedIntTy);
 
   // TODO: the "order" argument type is "int", not int32. So
   // getInt32Ty may be wrong if the arch uses e.g. 16-bit ints.
   ConstantInt *SizeVal64 = ConstantInt::get(Type::getInt64Ty(Ctx), Size);
   assert(Ordering != AtomicOrdering::NotAtomic && "expect atomic MO");
   Constant *OrderingVal =
       ConstantInt::get(Type::getInt32Ty(Ctx), (int)toCABI(Ordering));
   Constant *Ordering2Val = nullptr;

llvm/lib/IR/Instructions.cpp

AtomicCmpXchgInst::AtomicCmpXchgInst(Value *Ptr, Value *Cmp, Value *NewVal,
                                      BasicBlock *InsertAtEnd)
     : Instruction(
           StructType::get(Cmp->getType(), Type::getInt1Ty(Cmp->getContext())),
           AtomicCmpXchg, OperandTraits<AtomicCmpXchgInst>::op_begin(this),
           OperandTraits<AtomicCmpXchgInst>::operands(this), InsertAtEnd) {
   Init(Ptr, Cmp, NewVal, SuccessOrdering, FailureOrdering, SSID);
 }
 
+Align AtomicCmpXchgInst::getAlign() const {
+  // The default here is to assume it has NATURAL alignment, not
+  // DataLayout-specified alignment.
+  const DataLayout &DL = getModule()->getDataLayout();
+  return Align(DL.getTypeStoreSize(getCompareOperand()->getType()));
+}
+
 //===----------------------------------------------------------------------===//
 //                       AtomicRMWInst Implementation
 //===----------------------------------------------------------------------===//
 
 void AtomicRMWInst::Init(BinOp Operation, Value *Ptr, Value *Val,
                          AtomicOrdering Ordering,
                          SyncScope::ID SSID) {
   Op<0>() = Ptr;
case AtomicRMWInst::FSub:
     return "fsub";
   case AtomicRMWInst::BAD_BINOP:
     return "<invalid operation>";
   }
 
   llvm_unreachable("invalid atomicrmw operation");
 }
 
+Align AtomicRMWInst::getAlign() const {
+  // The default here is to assume it has NATURAL alignment, not
+  // DataLayout-specified alignment.
+  const DataLayout &DL = getModule()->getDataLayout();
+  return Align(DL.getTypeStoreSize(getValOperand()->getType()));
+}
+
 //===----------------------------------------------------------------------===//
 //                       FenceInst Implementation
 //===----------------------------------------------------------------------===//
 
 FenceInst::FenceInst(LLVMContext &C, AtomicOrdering Ordering,
                      SyncScope::ID SSID,
                      Instruction *InsertBefore)
   : Instruction(Type::getVoidTy(C), Fence, nullptr, 0, InsertBefore) {