LoadInst should store Align, not MaybeAlign.

[efriedma]

Summary

The fact that loads and stores can have the alignment missing is a constant source of confusion: code that usually works can break down in rare cases. So fix the LoadInst API so the alignment is never missing.

To reduce the number of changes required to make this work, IRBuilder and certain LoadInst constructors will grab the module's datalayout and compute the alignment automatically. This is the same alignment instcombine would eventually apply anyway; we're just doing it earlier. There's a minor risk that the way we're retrieving the datalayout could break out-of-tree code, but I don't think that's likely.

This is the last in a series of patches, so most of the necessary changes have already been merged.

[jdoerfert]

I'm in favor. The issues that arise due to confusion around the alignment are real.

[lebedev.ri]

I concur, this is in the same general direction as the troubles i was having with these in D76550.

[arsenm]

I don't think there should even be a MaybeAlign. There should always be an alignment in all relevant contexts

[gchatelet]

Thx a lot @efriedma!

The whole purpose of the effort behind Align/MaybeAlign was indeed to remove confusion (and bugs).
LLVM code is huge and I had to be conservative most of the time, navigating visually and trying to guess what was expected by interfaces. This is great to know that the LoadInst API needs a resolved alignment, definitely helps a lot!

I'm happy if we end up dropping MaybeAlign completely although I believe it's still necessary for some interfaces. The less the better obviously.

llvm/lib/Target/NVPTX/NVPTXLowerAggrCopies.cpp

123

Is this necessary? LI->getAlign() should suffice.

llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp

1012

auto Alignment = LI.getAlign();

llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp

559

Align LoadAlignment = FirstLI->getAlignment();

587

LoadAlignment = std::min(LoadAlignment, LI->getAlign());

[courbet]

llvm/lib/CodeGen/AtomicExpandPass.cpp

1380 ↗

(On Diff #255005)

Is it always the case that the required alignment is the size of the type ? Shouldn't we ask the DataLayout ?

[gchatelet]

Do you want me to take over and implement the change @efriedma?

[efriedma]

If you want to take this over, sure, go for it. :)

llvm/include/llvm/IR/IRBuilder.h

1716 ↗

(On Diff #255005)

This should use getValueOrABITypeAlignment

1731 ↗

(On Diff #255005)

This should use getValueOrABITypeAlignment

1747 ↗

(On Diff #255005)

This should use getValueOrABITypeAlignment

llvm/include/llvm/IR/Instructions.h

200

(Note this is currently based on D76269; master has some extra constructors. I'll try to get that merged soon.)

llvm/lib/AsmParser/LLParser.cpp

7004 ↗

(On Diff #255005)

This should use getValueOrABITypeAlignment

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

4830 ↗

(On Diff #255005)

This should use getValueOrABITypeAlignment

4871 ↗

(On Diff #255005)

This should use getValueOrABITypeAlignment

llvm/lib/CodeGen/AtomicExpandPass.cpp

1380 ↗

(On Diff #255005)

Currently atomicrmw assumes the pointer is naturally aligned, i.e. has alignment equal to the alignment of the type.

If we add explicit alignment to atomicrmw at some point, we would use that here.

The ABI alignment isn't relevant here.

llvm/lib/Target/NVPTX/NVPTXLowerAggrCopies.cpp

123

If you commit it separately, just "LI->getAlign()" will fail to build. If you commit it together, sure, it's redundant.

llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp

587

Again, if you commit it separately, just "LI->getAlign()" will fail to build. If you commit it together, sure, it's redundant.

[gchatelet]

In D77454#1964717, @efriedma wrote:

If you want to take this over, sure, go for it. :)

I wanted to but unfortunately I've to take that back. I'll be AFK for several weeks :-/

[efriedma]

Rebased. I landed a bunch of the trivial non-functional changes.

[jdoerfert]

Sorry for the wait. Some comments, mostly nits, some questions.

llvm/include/llvm/IR/IRBuilder.h

1747 ↗

(On Diff #255005)

Why doesn't it ?

llvm/include/llvm/IR/Instructions.h

184

Why do we want to remove the default values?

llvm/lib/IR/Instructions.cpp

1330

Nit: getModule

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp

2318–2319

Unrelated

llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp

1012

I'm against auto here

llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp

558

On first glance I'm not sure if why the below algorithm would not work if the alignment of all loads is "false".

llvm/lib/Transforms/Scalar/SROA.cpp

2572 ↗

(On Diff #255564)

I don't understand the change here and above tbh.

llvm/lib/Transforms/Utils/CodeExtractor.cpp

1174 ↗

(On Diff #255564)

Unrelated, just commit his.

[efriedma]

Rebase. Address review comments.

This is based on D78403. Regression tests should pass now.

[efriedma]

llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp

558

It would, but the logic is more complicated.

I just simplified this to assume loads have alignment set.

llvm/lib/Transforms/Scalar/SROA.cpp

2572 ↗

(On Diff #255564)

getSliceAlign() has code to try to avoid explicitly setting the alignment on load and store instructions, if it would be the same as the ABI alignment of the operation. This is complete nonsense, of course; it's actually a no-op on trunk because CreateAlignedLoad will recompute exactly the same alignment anyway.

[efriedma]

llvm/include/llvm/IR/Instructions.h

184

The four constructors that don't explicitly specify an alignment now grab the DataLayout from the last argument. So it can't be null.

[jdoerfert]

Looks much better, two comments below.

llvm/include/llvm/IR/Instructions.h

184

Makes sense. TBH, I would make it a reference so it breaks at compile time not runtime when null is passed explicitly.

llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp

558

thx!

llvm/lib/Transforms/Scalar/SROA.cpp

2572 ↗

(On Diff #255564)

Can you create a patch to remove this logic?

[efriedma]

llvm/include/llvm/IR/Instructions.h

184

Maybe it should be a reference... but I I don't really want to sign up to change every constructor for every Instruction to pass a reference instead of a pointer.

llvm/lib/Transforms/Scalar/SROA.cpp

2572 ↗

(On Diff #255564)

Split off the SROA changes into D78403.

[jdoerfert]

I'll accept tomorrow if no one raises a concern.

llvm/include/llvm/IR/Instructions.h

184

Yeah... Interesting that no in-tree user passes a nullptr. Maybe people don't do that.

[gchatelet]

Is this patch still supposed to land? If so can you format and rebase?

llvm/lib/Transforms/Scalar/SROA.cpp

1278 ↗

(On Diff #259385)

rename Align to Alignment and get rid of llvm::

[jdoerfert]

Forgot about this. LGTM, after the comments are addressed.

[efriedma]

Rebased, fixed clang-format.

llvm/include/llvm/IR/Instructions.h

 
 protected:
   // Note: Instruction needs to be a friend here to call cloneImpl.
   friend class Instruction;
 
   LoadInst *cloneImpl() const;
 
 public:
-  LoadInst(Type *Ty, Value *Ptr, const Twine &NameStr = "",
-           Instruction *InsertBefore = nullptr);
+  LoadInst(Type *Ty, Value *Ptr, const Twine &NameStr,
+           Instruction *InsertBefore);
   LoadInst(Type *Ty, Value *Ptr, const Twine &NameStr, BasicBlock *InsertAtEnd);
   LoadInst(Type *Ty, Value *Ptr, const Twine &NameStr, bool isVolatile,
-           Instruction *InsertBefore = nullptr);
+           Instruction *InsertBefore);
   LoadInst(Type *Ty, Value *Ptr, const Twine &NameStr, bool isVolatile,
            BasicBlock *InsertAtEnd);
   LoadInst(Type *Ty, Value *Ptr, const Twine &NameStr, bool isVolatile,
-           MaybeAlign Align, Instruction *InsertBefore = nullptr);
+           Align Align, Instruction *InsertBefore = nullptr);
   LoadInst(Type *Ty, Value *Ptr, const Twine &NameStr, bool isVolatile,
-           MaybeAlign Align, BasicBlock *InsertAtEnd);
+           Align Align, BasicBlock *InsertAtEnd);
   LoadInst(Type *Ty, Value *Ptr, const Twine &NameStr, bool isVolatile,
-           MaybeAlign Align, AtomicOrdering Order,
+           Align Align, AtomicOrdering Order,
            SyncScope::ID SSID = SyncScope::System,
            Instruction *InsertBefore = nullptr);
   LoadInst(Type *Ty, Value *Ptr, const Twine &NameStr, bool isVolatile,
-           MaybeAlign Align, AtomicOrdering Order, SyncScope::ID SSID,
+           Align Align, AtomicOrdering Order, SyncScope::ID SSID,
            BasicBlock *InsertAtEnd);
 
   /// Return true if this is a load from a volatile memory location.
   bool isVolatile() const { return getSubclassDataFromInstruction() & 1; }
 
   /// Specify whether this is a volatile load or not.
   void setVolatile(bool V) {
     setInstructionSubclassData((getSubclassDataFromInstruction() & ~1) |
                                (V ? 1 : 0));
   }
 
   /// Return the alignment of the access that is being performed.
   /// FIXME: Remove this function once transition to Align is over.
   /// Use getAlign() instead.
-  unsigned getAlignment() const {
-    if (const auto MA = getAlign())
-      return MA->value();
-    return 0;
-  }
+  unsigned getAlignment() const { return getAlign().value(); }
 
   /// Return the alignment of the access that is being performed.
-  MaybeAlign getAlign() const {
-    return decodeMaybeAlign((getSubclassDataFromInstruction() >> 1) & 31);
+  Align getAlign() const {
+    return *decodeMaybeAlign((getSubclassDataFromInstruction() >> 1) & 31);
   }
 
-  void setAlignment(MaybeAlign Alignment);
+  void setAlignment(Align Alignment);
 
   /// Returns the ordering constraint of this load instruction.
   AtomicOrdering getOrdering() const {
     return AtomicOrdering((getSubclassDataFromInstruction() >> 7) & 7);
   }
 
   /// Sets the ordering constraint of this load instruction.  May not be Release
   /// or AcquireRelease.

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

void SelectionDAGBuilder::visitAtomicLoad(const LoadInst &I) {
   if (!TLI.supportsUnalignedAtomics() &&
       I.getAlignment() < MemVT.getSizeInBits() / 8)
     report_fatal_error("Cannot generate unaligned atomic load");
 
   auto Flags = TLI.getLoadMemOperandFlags(I, DAG.getDataLayout());
 
   MachineMemOperand *MMO = DAG.getMachineFunction().getMachineMemOperand(
       MachinePointerInfo(I.getPointerOperand()), Flags, MemVT.getStoreSize(),
-      *I.getAlign(), AAMDNodes(), nullptr, SSID, Order);
+      I.getAlign(), AAMDNodes(), nullptr, SSID, Order);
 
   InChain = TLI.prepareVolatileOrAtomicLoad(InChain, dl, DAG);
 
   SDValue Ptr = getValue(I.getPointerOperand());
 
   if (TLI.lowerAtomicLoadAsLoadSDNode(I)) {
     // TODO: Once this is better exercised by tests, it should be merged with
     // the normal path for loads to prevent future divergence.

llvm/lib/IR/Core.cpp

 
 void LLVMSetAlignment(LLVMValueRef V, unsigned Bytes) {
   Value *P = unwrap<Value>(V);
   if (GlobalObject *GV = dyn_cast<GlobalObject>(P))
     GV->setAlignment(MaybeAlign(Bytes));
   else if (AllocaInst *AI = dyn_cast<AllocaInst>(P))
     AI->setAlignment(MaybeAlign(Bytes));
   else if (LoadInst *LI = dyn_cast<LoadInst>(P))
-    LI->setAlignment(MaybeAlign(Bytes));
+    LI->setAlignment(Align(Bytes));
   else if (StoreInst *SI = dyn_cast<StoreInst>(P))
     SI->setAlignment(MaybeAlign(Bytes));
   else
     llvm_unreachable(
         "only GlobalValue, AllocaInst, LoadInst and StoreInst have alignment");
 }
 
 LLVMValueMetadataEntry *LLVMGlobalCopyAllMetadata(LLVMValueRef Value,

llvm/lib/IR/Instructions.cpp

 
 void LoadInst::AssertOK() {
   assert(getOperand(0)->getType()->isPointerTy() &&
          "Ptr must have pointer type.");
   assert(!(isAtomic() && getAlignment() == 0) &&
          "Alignment required for atomic load");
 }
 
+Align computeLoadAlign(Type *Ty, BasicBlock *BB) {
+  const DataLayout &DL = BB->getModule()->getDataLayout();
+  return DL.getABITypeAlign(Ty);
+}
+
+Align computeLoadAlign(Type *Ty, Instruction *I) {
+  return computeLoadAlign(Ty, I->getParent());
+}
+
 LoadInst::LoadInst(Type *Ty, Value *Ptr, const Twine &Name,
                    Instruction *InsertBef)
     : LoadInst(Ty, Ptr, Name, /*isVolatile=*/false, InsertBef) {}
 
 LoadInst::LoadInst(Type *Ty, Value *Ptr, const Twine &Name,
                    BasicBlock *InsertAE)
     : LoadInst(Ty, Ptr, Name, /*isVolatile=*/false, InsertAE) {}
 
 LoadInst::LoadInst(Type *Ty, Value *Ptr, const Twine &Name, bool isVolatile,
                    Instruction *InsertBef)
-    : LoadInst(Ty, Ptr, Name, isVolatile, /*Align=*/None, InsertBef) {}
+    : LoadInst(Ty, Ptr, Name, isVolatile, computeLoadAlign(Ty, InsertBef),
+               InsertBef) {}
 
 LoadInst::LoadInst(Type *Ty, Value *Ptr, const Twine &Name, bool isVolatile,
                    BasicBlock *InsertAE)
-    : LoadInst(Ty, Ptr, Name, isVolatile, /*Align=*/None, InsertAE) {}
+    : LoadInst(Ty, Ptr, Name, isVolatile, computeLoadAlign(Ty, InsertAE),
+               InsertAE) {}
 
 LoadInst::LoadInst(Type *Ty, Value *Ptr, const Twine &Name, bool isVolatile,
-                   MaybeAlign Align, Instruction *InsertBef)
+                   Align Align, Instruction *InsertBef)
     : LoadInst(Ty, Ptr, Name, isVolatile, Align, AtomicOrdering::NotAtomic,
                SyncScope::System, InsertBef) {}
 
 LoadInst::LoadInst(Type *Ty, Value *Ptr, const Twine &Name, bool isVolatile,
-                   MaybeAlign Align, BasicBlock *InsertAE)
+                   Align Align, BasicBlock *InsertAE)
     : LoadInst(Ty, Ptr, Name, isVolatile, Align, AtomicOrdering::NotAtomic,
                SyncScope::System, InsertAE) {}
 
 LoadInst::LoadInst(Type *Ty, Value *Ptr, const Twine &Name, bool isVolatile,
-                   MaybeAlign Align, AtomicOrdering Order, SyncScope::ID SSID,
+                   Align Align, AtomicOrdering Order, SyncScope::ID SSID,
                    Instruction *InsertBef)
     : UnaryInstruction(Ty, Load, Ptr, InsertBef) {
   assert(Ty == cast<PointerType>(Ptr->getType())->getElementType());
   setVolatile(isVolatile);
-  setAlignment(MaybeAlign(Align));
+  setAlignment(Align);
   setAtomic(Order, SSID);
   AssertOK();
   setName(Name);
 }
 
 LoadInst::LoadInst(Type *Ty, Value *Ptr, const Twine &Name, bool isVolatile,
-                   MaybeAlign Align, AtomicOrdering Order, SyncScope::ID SSID,
+                   Align Align, AtomicOrdering Order, SyncScope::ID SSID,
                    BasicBlock *InsertAE)
     : UnaryInstruction(Ty, Load, Ptr, InsertAE) {
   assert(Ty == cast<PointerType>(Ptr->getType())->getElementType());
   setVolatile(isVolatile);
   setAlignment(Align);
   setAtomic(Order, SSID);
   AssertOK();
   setName(Name);
 }
 
-void LoadInst::setAlignment(MaybeAlign Align) {
-  assert((!Align || *Align <= MaximumAlignment) &&
+void LoadInst::setAlignment(Align Align) {
+  assert(Align <= MaximumAlignment &&
          "Alignment is greater than MaximumAlignment!");
   setInstructionSubclassData((getSubclassDataFromInstruction() & ~(31 << 1)) |
                              (encode(Align) << 1));
   assert(getAlign() == Align && "Alignment representation error!");
 }
 
 //===----------------------------------------------------------------------===//
 //                           StoreInst Implementation
AllocaInst *Result =
                      (Value *)getOperand(0), MaybeAlign(getAlignment()));
   Result->setUsedWithInAlloca(isUsedWithInAlloca());
   Result->setSwiftError(isSwiftError());
   return Result;
 }
 
 LoadInst *LoadInst::cloneImpl() const {
   return new LoadInst(getType(), getOperand(0), Twine(), isVolatile(),
-                      MaybeAlign(getAlignment()), getOrdering(),
-                      getSyncScopeID());
+                      getAlign(), getOrdering(), getSyncScopeID());
 }
 
 StoreInst *StoreInst::cloneImpl() const {
   return new StoreInst(getOperand(0), getOperand(1), isVolatile(),
                        MaybeAlign(getAlignment()), getOrdering(),
                        getSyncScopeID());
 }
 

llvm/lib/Target/NVPTX/NVPTXLowerAggrCopies.cpp

for (LoadInst *LI : AggrLoads) {
     Value *DstAddr = SI->getOperand(1);
     unsigned NumLoads = DL.getTypeStoreSize(LI->getType());
     ConstantInt *CopyLen =
         ConstantInt::get(Type::getInt32Ty(Context), NumLoads);
 
     createMemCpyLoopKnownSize(/* ConvertedInst */ SI,
                               /* SrcAddr */ SrcAddr, /* DstAddr */ DstAddr,
                               /* CopyLen */ CopyLen,
-                              /* SrcAlign */ LI->getAlign().valueOrOne(),
+                              /* SrcAlign */ LI->getAlign(),
                               /* DestAlign */ SI->getAlign().valueOrOne(),
                               /* SrcIsVolatile */ LI->isVolatile(),
                               /* DstIsVolatile */ SI->isVolatile(), TTI);
 
     SI->eraseFromParent();
     LI->eraseFromParent();
   }
 
   // Transform mem* intrinsic calls.
   for (MemIntrinsic *MemCall : MemCalls) {
     if (MemCpyInst *Memcpy = dyn_cast<MemCpyInst>(MemCall)) {
       expandMemCpyAsLoop(Memcpy, TTI);
     } else if (MemMoveInst *Memmove = dyn_cast<MemMoveInst>(MemCall)) {
       expandMemMoveAsLoop(Memmove);
     } else if (MemSetInst *Memset = dyn_cast<MemSetInst>(MemCall)) {

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp

if (M->getNumOperands() == 3 && M->getOperand(0) &&
         M->getOperand(2) && isa<MDNode>(M->getOperand(2)))
       CopyMD = cast<MDNode>(M->getOperand(2));
   }
 
   Value *Src = Builder.CreateBitCast(MI->getArgOperand(1), NewSrcPtrTy);
   Value *Dest = Builder.CreateBitCast(MI->getArgOperand(0), NewDstPtrTy);
   LoadInst *L = Builder.CreateLoad(IntType, Src);
   // Alignment from the mem intrinsic will be better, so use it.
-  L->setAlignment(
-      MaybeAlign(CopySrcAlign)); // FIXME: Check if we can use Align instead.
+  L->setAlignment(*CopySrcAlign);
   if (CopyMD)
     L->setMetadata(LLVMContext::MD_tbaa, CopyMD);
   MDNode *LoopMemParallelMD =
     MI->getMetadata(LLVMContext::MD_mem_parallel_loop_access);
   if (LoopMemParallelMD)
     L->setMetadata(LLVMContext::MD_mem_parallel_loop_access, LoopMemParallelMD);
   MDNode *AccessGroupMD = MI->getMetadata(LLVMContext::MD_access_group);
   if (AccessGroupMD)
case Intrinsic::maximum: {
 
     Value *ExtSrc0;
     Value *ExtSrc1;
 
     // minnum (fpext x), (fpext y) -> minnum x, y
     // maxnum (fpext x), (fpext y) -> maxnum x, y
     if (match(II->getArgOperand(0), m_OneUse(m_FPExt(m_Value(ExtSrc0)))) &&
         match(II->getArgOperand(1), m_OneUse(m_FPExt(m_Value(ExtSrc1)))) &&
         ExtSrc0->getType() == ExtSrc1->getType()) {
       Function *F = Intrinsic::getDeclaration(
           II->getModule(), II->getIntrinsicID(), {ExtSrc0->getType()});
       CallInst *NewCall = Builder.CreateCall(F, { ExtSrc0, ExtSrc1 });
       NewCall->copyFastMathFlags(II);
       NewCall->takeName(II);
       return new FPExtInst(NewCall, II->getType());
     }
 
     break;
Instruction *InstCombiner::visitCallInst(CallInst &CI) {
   }
   case Intrinsic::ppc_altivec_lvx:
   case Intrinsic::ppc_altivec_lvxl:
     // Turn PPC lvx -> load if the pointer is known aligned.
     if (getOrEnforceKnownAlignment(II->getArgOperand(0), Align(16), DL, II, &AC,
                                    &DT) >= 16) {
       Value *Ptr = Builder.CreateBitCast(II->getArgOperand(0),
                                          PointerType::getUnqual(II->getType()));
-      return new LoadInst(II->getType(), Ptr);
+      return new LoadInst(II->getType(), Ptr, "", false, Align(16));
     }
     break;
   case Intrinsic::ppc_vsx_lxvw4x:
   case Intrinsic::ppc_vsx_lxvd2x: {
     // Turn PPC VSX loads into normal loads.
     Value *Ptr = Builder.CreateBitCast(II->getArgOperand(0),
                                        PointerType::getUnqual(II->getType()));
     return new LoadInst(II->getType(), Ptr, Twine(""), false, Align(1));
case Intrinsic::ppc_qpx_qvlfs:
     }
     break;
   case Intrinsic::ppc_qpx_qvlfd:
     // Turn PPC QPX qvlfd -> load if the pointer is known aligned.
     if (getOrEnforceKnownAlignment(II->getArgOperand(0), Align(32), DL, II, &AC,
                                    &DT) >= 32) {
       Value *Ptr = Builder.CreateBitCast(II->getArgOperand(0),
                                          PointerType::getUnqual(II->getType()));
-      return new LoadInst(II->getType(), Ptr);
+      return new LoadInst(II->getType(), Ptr, "", false, Align(32));
     }
     break;
   case Intrinsic::ppc_qpx_qvstfs:
     // Turn PPC QPX qvstfs -> store if the pointer is known aligned.
     if (getOrEnforceKnownAlignment(II->getArgOperand(1), Align(16), DL, II, &AC,
                                    &DT) >= 16) {
       Type *VTy = VectorType::get(
           Builder.getFloatTy(),

llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp

 
 void PointerReplacer::replace(Instruction *I) {
   if (getReplacement(I))
     return;
 
   if (auto *LT = dyn_cast<LoadInst>(I)) {
     auto *V = getReplacement(LT->getPointerOperand());
     assert(V && "Operand not replaced");
-    auto *NewI = new LoadInst(I->getType(), V);
+    auto *NewI = new LoadInst(I->getType(), V, "", false,
+                              IC.getDataLayout().getABITypeAlign(I->getType()));
     NewI->takeName(LT);
     IC.InsertNewInstWith(NewI, *LT);
     IC.replaceInstUsesWith(*LT, NewI);
     WorkMap[LT] = NewI;
   } else if (auto *GEP = dyn_cast<GetElementPtrInst>(I)) {
     auto *V = getReplacement(GEP->getPointerOperand());
     assert(V && "Operand not replaced");
     SmallVector<Value *, 8> Indices;
if (Op->hasOneUse()) {
     // Note that we cannot do the transformation unless we know that the
     // introduced loads cannot trap!  Something like this is valid as long as
     // the condition is always false: load (select bool %C, int* null, int* %G),
     // but it would not be valid if we transformed it to load from null
     // unconditionally.
     //
     if (SelectInst *SI = dyn_cast<SelectInst>(Op)) {
       // load (select (Cond, &V1, &V2))  --> select(Cond, load &V1, load &V2).
-      const MaybeAlign Alignment(LI.getAlignment());
+      Align Alignment = LI.getAlign();
       if (isSafeToLoadUnconditionally(SI->getOperand(1), LI.getType(),
                                       Alignment, DL, SI) &&
           isSafeToLoadUnconditionally(SI->getOperand(2), LI.getType(),
                                       Alignment, DL, SI)) {
         LoadInst *V1 =
             Builder.CreateLoad(LI.getType(), SI->getOperand(1),
                                SI->getOperand(1)->getName() + ".val");
         LoadInst *V2 =

llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp

if (FirstLI->isAtomic())
     return nullptr;
 
   // When processing loads, we need to propagate two bits of information to the
   // sunk load: whether it is volatile, and what its alignment is.  We currently
   // don't sink loads when some have their alignment specified and some don't.
   // visitLoadInst will propagate an alignment onto the load when TD is around,
   // and if TD isn't around, we can't handle the mixed case.
   bool isVolatile = FirstLI->isVolatile();
-  MaybeAlign LoadAlignment(FirstLI->getAlignment());
+  Align LoadAlignment = FirstLI->getAlign();
   unsigned LoadAddrSpace = FirstLI->getPointerAddressSpace();
 
   // We can't sink the load if the loaded value could be modified between the
   // load and the PHI.
   if (FirstLI->getParent() != PN.getIncomingBlock(0) ||
       !isSafeAndProfitableToSinkLoad(FirstLI))
     return nullptr;
 
   // If the PHI is of volatile loads and the load block has multiple
   // successors, sinking it would remove a load of the volatile value from
for (unsigned i = 1, e = PN.getNumIncomingValues(); i != e; ++i) {
     // We can't sink the load if the loaded value could be modified between
     // the load and the PHI.
     if (LI->isVolatile() != isVolatile ||
         LI->getParent() != PN.getIncomingBlock(i) ||
         LI->getPointerAddressSpace() != LoadAddrSpace ||
         !isSafeAndProfitableToSinkLoad(LI))
       return nullptr;
 
-    // If some of the loads have an alignment specified but not all of them,
-    // we can't do the transformation.
-    if ((LoadAlignment.hasValue()) != (LI->getAlignment() != 0))
-      return nullptr;
-
-    LoadAlignment = std::min(LoadAlignment, MaybeAlign(LI->getAlignment()));
+    LoadAlignment = std::min(LoadAlignment, Align(LI->getAlign()));
 
     // If the PHI is of volatile loads and the load block has multiple
     // successors, sinking it would remove a load of the volatile value from
     // the path through the other successor.
     if (isVolatile &&
         LI->getParent()->getTerminator()->getNumSuccessors() != 1)
       return nullptr;
   }

llvm/lib/Transforms/Scalar/AlignmentFromAssumptions.cpp

for (User *J : AAPtr->users()) {
     if (J == ACall)
       continue;
 
     if (Instruction *K = dyn_cast<Instruction>(J))
       if (isValidAssumeForContext(ACall, K, DT))
         WorkList.push_back(K);
   }
 
+  const DataLayout &DL = SE->getDataLayout();
   while (!WorkList.empty()) {
     Instruction *J = WorkList.pop_back_val();
     if (LoadInst *LI = dyn_cast<LoadInst>(J)) {
       Align NewAlignment = getNewAlignment(AASCEV, AlignSCEV, OffSCEV,
                                            LI->getPointerOperand(), SE);
-
-      if (NewAlignment > *LI->getAlign()) {
+      Align OldAlignment =
+          DL.getValueOrABITypeAlignment(LI->getAlign(), LI->getType());
+      if (NewAlignment > OldAlignment) {
         LI->setAlignment(NewAlignment);
         ++NumLoadAlignChanged;
       }
     } else if (StoreInst *SI = dyn_cast<StoreInst>(J)) {
       Align NewAlignment = getNewAlignment(AASCEV, AlignSCEV, OffSCEV,
                                            SI->getPointerOperand(), SE);
-
-      if (NewAlignment > *SI->getAlign()) {
+      Align OldAlignment = DL.getValueOrABITypeAlignment(
+          SI->getAlign(), SI->getOperand(0)->getType());
+      if (NewAlignment > OldAlignment) {
         SI->setAlignment(NewAlignment);
         ++NumStoreAlignChanged;
       }
     } else if (MemIntrinsic *MI = dyn_cast<MemIntrinsic>(J)) {
       Align NewDestAlignment =
           getNewAlignment(AASCEV, AlignSCEV, OffSCEV, MI->getDest(), SE);
 
       LLVM_DEBUG(dbgs() << "\tmem inst: " << DebugStr(NewDestAlignment)

llvm/lib/Transforms/Scalar/GVNHoist.cpp

void makeGepsAvailable(Instruction *Repl, BasicBlock *HoistPt,
     }
 
     // Replace uses of Gep with ClonedGep in Repl.
     Repl->replaceUsesOfWith(Gep, ClonedGep);
   }
 
   void updateAlignment(Instruction *I, Instruction *Repl) {
     if (auto *ReplacementLoad = dyn_cast<LoadInst>(Repl)) {
-      ReplacementLoad->setAlignment(MaybeAlign(std::min(
-          ReplacementLoad->getAlignment(), cast<LoadInst>(I)->getAlignment())));
+      ReplacementLoad->setAlignment(
+          std::min(ReplacementLoad->getAlign(), cast<LoadInst>(I)->getAlign()));
       ++NumLoadsRemoved;
     } else if (auto *ReplacementStore = dyn_cast<StoreInst>(Repl)) {
       ReplacementStore->setAlignment(
           MaybeAlign(std::min(ReplacementStore->getAlignment(),
                               cast<StoreInst>(I)->getAlignment())));
       ++NumStoresRemoved;
     } else if (auto *ReplacementAlloca = dyn_cast<AllocaInst>(Repl)) {
       ReplacementAlloca->setAlignment(