Make Value::getPointerAlignment() return an Align, not a MaybeAlign.

[efriedma]

Summary

If we don't know anything about the alignment of a pointer, Align(1) is still correct: all pointers are at least 1-byte aligned.

The changes to Loads.cpp are messy; I'm not sure what the original intent was. It looks like the logic in question originates from D9791.

[jdoerfert]

Everything except the Loads.cpp stuff LGTM.

llvm/lib/Analysis/Loads.cpp

40

As you noted, this seems wrong. I think this allows wrong results for llvm::isDereferenceableAndAlignedPointer queries. What happens if you remove this boosting?

---

From D9791:

I assume that pointers without explicit alignment have ABI alignment.

I think this is OK for pointers accessed by loads without specified alignment but not for "pointers" in the general sense. If you look at the AttributorAttributes.cpp change, the function there does something similar to the getBaseAlign on the left but it only does so for pointers accessed without specified alignment. (= I think I came to this conclusion for the second time.)

[efriedma]

Show what happens if I remove the weird special case from Loads.cpp

[efriedma]

llvm/lib/Analysis/Loads.cpp

40

The simplest way to show what happens is to just post the patch, so I did that. The issue primarily involves pointers marked with the dereferenceable metadata/attribute; we used to assume they were naturally aligned, but now we don't.

Frontends adding dereferenceable attributes/metadata should also add the corresponding alignment attributes/metadata, but I'm not sure that's actually happening consistently in practice.

[jdoerfert]

llvm/lib/Analysis/Loads.cpp

40

I still believe the code was incorrect so I'm in favor of removing it. We might need to improve other places but that seems doable. Let's wait for a while to see if others want to chime in. (@fhahn, @lebedev.ri, @spatel, @nikic, @arsenm, @reames, @nlopes)

[nikic]

llvm/lib/Analysis/Loads.cpp

40

Agree that the current behavior is incorrect and we really need to do something about this, because it will block opaque pointers as well.

I think Eli is right though and Clang doesn't emit the necessary alignment attributes here right now, so that probably needs to be addressed first.

[efriedma]

llvm/lib/Analysis/Loads.cpp

40

I think D80166 takes care of clang. I can't find any other code that needs to be updated.

[efriedma]

llvm/lib/Analysis/Loads.cpp

40

Just merged D80166.

[jdoerfert]

I still think this is the right thing to do and the changes are fine. LGTM. (Let's wait again a little while or until we get another person to look at this.)

Nit: Commit message should be updated. Maybe refer to the clang patch and mention the loads.cpp bug.

[nikic]

LGTM as well.

I'd suggest adding a release note suggesting frontends to make sure they emit align attributes/metadata when emitting dereferenceable attributes/metadata.

llvm/include/llvm/IR/Value.h

#include "llvm/IR/Value.def"
   /// dereferenceable up to the returned number of bytes.
   uint64_t getPointerDereferenceableBytes(const DataLayout &DL,
                                           bool &CanBeNull) const;
 
   /// Returns an alignment of the pointer value.
   ///
   /// Returns an alignment which is either specified explicitly, e.g. via
   /// align attribute of a function argument, or guaranteed by DataLayout.
-  MaybeAlign getPointerAlignment(const DataLayout &DL) const;
+  Align getPointerAlignment(const DataLayout &DL) const;
 
   /// Translate PHI node to its predecessor from the given basic block.
   ///
   /// If this value is a PHI node with CurBB as its parent, return the value in
   /// the PHI node corresponding to PredBB.  If not, return ourself.  This is
   /// useful if you want to know the value something has in a predecessor
   /// block.
   const Value *DoPHITranslation(const BasicBlock *CurBB,

llvm/lib/Analysis/Loads.cpp

 #include "llvm/IR/IntrinsicInst.h"
 #include "llvm/IR/LLVMContext.h"
 #include "llvm/IR/Module.h"
 #include "llvm/IR/Operator.h"
 #include "llvm/IR/Statepoint.h"
 
 using namespace llvm;
 
-static MaybeAlign getBaseAlign(const Value *Base, const DataLayout &DL) {
-  if (const MaybeAlign PA = Base->getPointerAlignment(DL))
-    return *PA;
-  Type *const Ty = Base->getType()->getPointerElementType();
-  if (!Ty->isSized())
-    return None;
-  return Align(DL.getABITypeAlignment(Ty));
-}
-
 static bool isAligned(const Value *Base, const APInt &Offset, Align Alignment,
                       const DataLayout &DL) {
-  if (MaybeAlign BA = getBaseAlign(Base, DL)) {
-    const APInt APBaseAlign(Offset.getBitWidth(), BA->value());
-    const APInt APAlign(Offset.getBitWidth(), Alignment.value());
-    assert(APAlign.isPowerOf2() && "must be a power of 2!");
-    return APBaseAlign.uge(APAlign) && !(Offset & (APAlign - 1));
-  }
-  return false;
+  Align BA = Base->getPointerAlignment(DL);
+  if (BA == 1 && !isa<AllocaInst>(Base) && !isa<Constant>(Base)) {
+    // If we determined the pointer was unaligned, boost the alignment
+    // based on the pointer element type.
+    // FIXME: This seems dubious, but the regression tests expect it.
+    Type *Ty = Base->getType()->getPointerElementType();
+    if (Ty->isSized())
+      BA = DL.getABITypeAlign(Ty);
+  }
+  return BA >= Alignment && !(Offset & (APAlign - 1));
 }
 
 /// Test if V is always a pointer to allocated and suitably aligned memory for
 /// a simple load or store.
 static bool isDereferenceableAndAlignedPointer(
     const Value *V, Align Alignment, const APInt &Size, const DataLayout &DL,
     const Instruction *CtxI, const DominatorTree *DT,
     SmallPtrSetImpl<const Value *> &Visited, unsigned MaxDepth) {

llvm/lib/Analysis/ValueTracking.cpp

if (const GlobalAlias *GA = dyn_cast<GlobalAlias>(V)) {
     return;
   }
 
   if (const Operator *I = dyn_cast<Operator>(V))
     computeKnownBitsFromOperator(I, DemandedElts, Known, Depth, Q);
 
   // Aligned pointers have trailing zeros - refine Known.Zero set
   if (isa<PointerType>(V->getType())) {
-    const MaybeAlign Align = V->getPointerAlignment(Q.DL);
-    if (Align)
-      Known.Zero.setLowBits(countTrailingZeros(Align->value()));
+    Align Alignment = V->getPointerAlignment(Q.DL);
+    Known.Zero.setLowBits(countTrailingZeros(Alignment.value()));
   }
 
   // computeKnownBitsFromAssume strictly refines Known.
   // Therefore, we run them after computeKnownBitsFromOperator.
 
   // Check whether a nearby assume intrinsic can determine some known bits.
   computeKnownBitsFromAssume(V, Known, Depth, Q);
 

llvm/lib/CodeGen/ExpandMemCmp.cpp

 
 MemCmpExpansion::LoadPair MemCmpExpansion::getLoadPair(Type *LoadSizeType,
                                                        bool NeedsBSwap,
                                                        Type *CmpSizeType,
                                                        unsigned OffsetBytes) {
   // Get the memory source at offset `OffsetBytes`.
   Value *LhsSource = CI->getArgOperand(0);
   Value *RhsSource = CI->getArgOperand(1);
-  Align LhsAlign = LhsSource->getPointerAlignment(DL).valueOrOne();
-  Align RhsAlign = RhsSource->getPointerAlignment(DL).valueOrOne();
+  Align LhsAlign = LhsSource->getPointerAlignment(DL);
+  Align RhsAlign = RhsSource->getPointerAlignment(DL);
   if (OffsetBytes > 0) {
     auto *ByteType = Type::getInt8Ty(CI->getContext());
     LhsSource = Builder.CreateConstGEP1_64(
         ByteType, Builder.CreateBitCast(LhsSource, ByteType->getPointerTo()),
         OffsetBytes);
     RhsSource = Builder.CreateConstGEP1_64(
         ByteType, Builder.CreateBitCast(RhsSource, ByteType->getPointerTo()),
         OffsetBytes);

llvm/lib/IR/ConstantFold.cpp

case Instruction::And:
         // If and'ing the address of a global with a constant, fold it.
         if (CE1->getOpcode() == Instruction::PtrToInt &&
             isa<GlobalValue>(CE1->getOperand(0))) {
           GlobalValue *GV = cast<GlobalValue>(CE1->getOperand(0));
 
           MaybeAlign GVAlign;
 
           if (Module *TheModule = GV->getParent()) {
-            GVAlign = GV->getPointerAlignment(TheModule->getDataLayout());
+            const DataLayout &DL = TheModule->getDataLayout();
+            GVAlign = GV->getPointerAlignment(DL);
 
             // If the function alignment is not specified then assume that it
             // is 4.
             // This is dangerous; on x86, the alignment of the pointer
             // corresponds to the alignment of the function, but might be less
             // than 4 if it isn't explicitly specified.
             // However, a fix for this behaviour was reverted because it
             // increased code size (see https://reviews.llvm.org/D55115)
             // FIXME: This code should be deleted once existing targets have
             // appropriate defaults
-            if (!GVAlign && isa<Function>(GV))
+            if (isa<Function>(GV) && !DL.getFunctionPtrAlign())
               GVAlign = Align(4);
           } else if (isa<Function>(GV)) {
             // Without a datalayout we have to assume the worst case: that the
             // function pointer isn't aligned at all.
             GVAlign = llvm::None;
           } else {
             GVAlign = MaybeAlign(GV->getAlignment());
           }

llvm/lib/IR/Value.cpp

if (GV->getValueType()->isSized() && !GV->hasExternalWeakLinkage()) {
       // CanBeNull flag.
       DerefBytes = DL.getTypeStoreSize(GV->getValueType()).getFixedSize();
       CanBeNull = false;
     }
   }
   return DerefBytes;
 }
 
-MaybeAlign Value::getPointerAlignment(const DataLayout &DL) const {
+Align Value::getPointerAlignment(const DataLayout &DL) const {
   assert(getType()->isPointerTy() && "must be pointer");
   if (auto *GO = dyn_cast<GlobalObject>(this)) {
     if (isa<Function>(GO)) {
-      const MaybeAlign FunctionPtrAlign = DL.getFunctionPtrAlign();
+      Align FunctionPtrAlign = DL.getFunctionPtrAlign().valueOrOne();
       switch (DL.getFunctionPtrAlignType()) {
       case DataLayout::FunctionPtrAlignType::Independent:
         return FunctionPtrAlign;
       case DataLayout::FunctionPtrAlignType::MultipleOfFunctionAlign:
-        return std::max(FunctionPtrAlign, MaybeAlign(GO->getAlignment()));
+        return std::max(FunctionPtrAlign, GO->getAlign().valueOrOne());
       }
       llvm_unreachable("Unhandled FunctionPtrAlignType");
     }
     const MaybeAlign Alignment(GO->getAlignment());
     if (!Alignment) {
       if (auto *GVar = dyn_cast<GlobalVariable>(GO)) {
         Type *ObjectType = GVar->getValueType();
         if (ObjectType->isSized()) {
           // If the object is defined in the current Module, we'll be giving
           // it the preferred alignment. Otherwise, we have to assume that it
           // may only have the minimum ABI alignment.
           if (GVar->isStrongDefinitionForLinker())
-            return MaybeAlign(DL.getPreferredAlignment(GVar));
+            return Align(DL.getPreferredAlignment(GVar));
           else
             return DL.getABITypeAlign(ObjectType);
         }
       }
     }
-    return Alignment;
+    return Alignment.valueOrOne();
   } else if (const Argument *A = dyn_cast<Argument>(this)) {
     const MaybeAlign Alignment = A->getParamAlign();
     if (!Alignment && A->hasStructRetAttr()) {
       // An sret parameter has at least the ABI alignment of the return type.
       Type *EltTy = cast<PointerType>(A->getType())->getElementType();
       if (EltTy->isSized())
         return DL.getABITypeAlign(EltTy);
     }
-    return Alignment;
+    return Alignment.valueOrOne();
   } else if (const AllocaInst *AI = dyn_cast<AllocaInst>(this)) {
-    const MaybeAlign Alignment = AI->getAlign();
-    if (!Alignment) {
-      Type *AllocatedType = AI->getAllocatedType();
-      if (AllocatedType->isSized())
-        return MaybeAlign(DL.getPrefTypeAlignment(AllocatedType));
-    }
-    return Alignment;
+    return AI->getAlign();
   } else if (const auto *Call = dyn_cast<CallBase>(this)) {
-    const MaybeAlign Alignment = Call->getRetAlign();
+    MaybeAlign Alignment = Call->getRetAlign();
     if (!Alignment && Call->getCalledFunction())
-      return MaybeAlign(
-          Call->getCalledFunction()->getAttributes().getRetAlignment());
-    return Alignment;
+      Alignment = Call->getCalledFunction()->getAttributes().getRetAlignment();
+    return Alignment.valueOrOne();
   } else if (const LoadInst *LI = dyn_cast<LoadInst>(this)) {
     if (MDNode *MD = LI->getMetadata(LLVMContext::MD_align)) {
       ConstantInt *CI = mdconst::extract<ConstantInt>(MD->getOperand(0));
-      return MaybeAlign(CI->getLimitedValue());
+      return Align(CI->getLimitedValue());
     }
   } else if (auto *CstPtr = dyn_cast<Constant>(this)) {
     if (auto *CstInt = dyn_cast_or_null<ConstantInt>(ConstantExpr::getPtrToInt(
             const_cast<Constant *>(CstPtr), DL.getIntPtrType(getType()),
             /*OnlyIfReduced=*/true))) {
       size_t TrailingZeros = CstInt->getValue().countTrailingZeros();
       // While the actual alignment may be large, elsewhere we have
       // an arbitrary upper alignmet limit, so let's clamp to it.
       return Align(TrailingZeros < Value::MaxAlignmentExponent
                        ? uint64_t(1) << TrailingZeros
                        : Value::MaximumAlignment);
     }
   }
-  return llvm::None;
+  return Align(1);
 }
 
 const Value *Value::DoPHITranslation(const BasicBlock *CurBB,
                                      const BasicBlock *PredBB) const {
   auto *PN = dyn_cast<PHINode>(this);
   if (PN && PN->getParent() == CurBB)
     return PN->getIncomingValueForBlock(PredBB);
   return this;

llvm/lib/Target/AArch64/AArch64InstrInfo.td

 def : InstAlias<"prfm $Rt, [$Rn]", (PRFMui prfop:$Rt, GPR64sp:$Rn, 0)>;
 
 //---
 // (literal)
 
 def alignedglobal : PatLeaf<(iPTR iPTR:$label), [{
   if (auto *G = dyn_cast<GlobalAddressSDNode>(N)) {
     const DataLayout &DL = MF->getDataLayout();
-    MaybeAlign Align = G->getGlobal()->getPointerAlignment(DL);
-    return Align && *Align >= 4 && G->getOffset() % 4 == 0;
+    Align Align = G->getGlobal()->getPointerAlignment(DL);
+    return Align >= 4 && G->getOffset() % 4 == 0;
   }
   if (auto *C = dyn_cast<ConstantPoolSDNode>(N))
     return C->getAlign() >= 4 && C->getOffset() % 4 == 0;
   return false;
 }]>;
 
 def LDRWl : LoadLiteral<0b00, 0, GPR32z, "ldr",
   [(set GPR32z:$Rt, (load (AArch64adr alignedglobal:$label)))]>;

llvm/lib/Transforms/IPO/AttributorAttributes.cpp

struct AADereferenceableCallSiteReturned final
   /// See AbstractAttribute::trackStatistics()
   void trackStatistics() const override {
     STATS_DECLTRACK_CS_ATTR(dereferenceable);
   }
 };
 
 // ------------------------ Align Argument Attribute ------------------------
 
-/// \p Ptr is accessed so we can get alignment information if the ABI requires
-/// the element type to be aligned.
-static MaybeAlign getKnownAlignmentFromAccessedPtr(const Value *Ptr,
-                                                   const DataLayout &DL) {
-  MaybeAlign KnownAlignment = Ptr->getPointerAlignment(DL);
-  Type *ElementTy = Ptr->getType()->getPointerElementType();
-  if (ElementTy->isSized())
-    KnownAlignment = max(KnownAlignment, DL.getABITypeAlign(ElementTy));
-  return KnownAlignment;
-}
-
 static unsigned getKnownAlignForUse(Attributor &A,
                                     AbstractAttribute &QueryingAA,
                                     Value &AssociatedValue, const Use *U,
                                     const Instruction *I, bool &TrackUse) {
   // We need to follow common pointer manipulation uses to the accesses they
   // feed into.
   if (isa<CastInst>(I)) {
     // Follow all but ptr2int casts.
if (const auto *CB = dyn_cast<CallBase>(I)) {
     auto &AlignAA = A.getAAFor<AAAlign>(QueryingAA, IRP,
                                         /* TrackDependence */ false);
     MA = MaybeAlign(AlignAA.getKnownAlign());
   }
 
   const DataLayout &DL = A.getDataLayout();
   const Value *UseV = U->get();
   if (auto *SI = dyn_cast<StoreInst>(I)) {
-    if (SI->getPointerOperand() == UseV) {
-      if (unsigned SIAlign = SI->getAlignment())
-        MA = MaybeAlign(SIAlign);
-      else
-        MA = getKnownAlignmentFromAccessedPtr(UseV, DL);
-    }
+    if (SI->getPointerOperand() == UseV)
+      MA = SI->getAlign();
   } else if (auto *LI = dyn_cast<LoadInst>(I)) {
-    if (LI->getPointerOperand() == UseV) {
-      if (unsigned LIAlign = LI->getAlignment())
-        MA = MaybeAlign(LIAlign);
-      else
-        MA = getKnownAlignmentFromAccessedPtr(UseV, DL);
-    }
+    if (LI->getPointerOperand() == UseV)
+      MA = LI->getAlign();
   }
 
   if (!MA.hasValue() || MA <= 1)
     return 0;
 
   unsigned Alignment = MA->value();
   int64_t Offset;
 
void initialize(Attributor &A) override {
 
     Value &V = getAssociatedValue();
     // TODO: This is a HACK to avoid getPointerAlignment to introduce a ptr2int
     //       use of the function pointer. This was caused by D73131. We want to
     //       avoid this for function pointers especially because we iterate
     //       their uses and int2ptr is not handled. It is not a correctness
     //       problem though!
     if (!V.getType()->getPointerElementType()->isFunctionTy())
-      takeKnownMaximum(
-          V.getPointerAlignment(A.getDataLayout()).valueOrOne().value());
+      takeKnownMaximum(V.getPointerAlignment(A.getDataLayout()).value());
 
     if (getIRPosition().isFnInterfaceKind() &&
         (!getAnchorScope() ||
          !A.isFunctionIPOAmendable(*getAssociatedFunction()))) {
       indicatePessimisticFixpoint();
       return;
     }
 
for (const Use &U : AssociatedValue.uses()) {
                             "Number of times alignment added to a load");
             LoadStoreChanged = ChangeStatus::CHANGED;
           }
       }
     }
 
     ChangeStatus Changed = AAAlign::manifest(A);
 
-    MaybeAlign InheritAlign =
+    Align InheritAlign =
         getAssociatedValue().getPointerAlignment(A.getDataLayout());
-    if (InheritAlign.valueOrOne() >= getAssumedAlign())
+    if (InheritAlign >= getAssumedAlign())
       return LoadStoreChanged;
     return Changed | LoadStoreChanged;
   }
 
   // TODO: Provide a helper to determine the implied ABI alignment and check in
   //       the existing manifest method and a new one for AAAlignImpl that value
   //       to avoid making the alignment explicit if it did not improve.
 
struct AAAlignFloating : AAAlignImpl {
   ChangeStatus updateImpl(Attributor &A) override {
     const DataLayout &DL = A.getDataLayout();
 
     auto VisitValueCB = [&](Value &V, const Instruction *,
                             AAAlign::StateType &T, bool Stripped) -> bool {
       const auto &AA = A.getAAFor<AAAlign>(*this, IRPosition::value(V));
       if (!Stripped && this == &AA) {
         // Use only IR information if we did not strip anything.
-        const MaybeAlign PA = V.getPointerAlignment(DL);
-        T.takeKnownMaximum(PA ? PA->value() : 0);
+        Align PA = V.getPointerAlignment(DL);
+        T.takeKnownMaximum(PA.value());
         T.indicatePessimisticFixpoint();
       } else {
         // Use abstract attribute information.
         const AAAlign::StateType &DS =
             static_cast<const AAAlign::StateType &>(AA.getState());
         T ^= DS;
       }
       return T.isValidState();
struct AAAlignCallSiteArgument final : AAAlignFloating {
   ChangeStatus manifest(Attributor &A) override {
     // If the associated argument is involved in a must-tail call we give up
     // because we would need to keep the argument alignments of caller and
     // callee in-sync. Just does not seem worth the trouble right now.
     if (Argument *Arg = getAssociatedArgument())
       if (A.getInfoCache().isInvolvedInMustTailCall(*Arg))
         return ChangeStatus::UNCHANGED;
     ChangeStatus Changed = AAAlignImpl::manifest(A);
-    MaybeAlign InheritAlign =
+    Align InheritAlign =
         getAssociatedValue().getPointerAlignment(A.getDataLayout());
-    if (InheritAlign.valueOrOne() >= getAssumedAlign())
+    if (InheritAlign >= getAssumedAlign())
       Changed = ChangeStatus::UNCHANGED;
     return Changed;
   }
 
   /// See AbstractAttribute::updateImpl(Attributor &A).
   ChangeStatus updateImpl(Attributor &A) override {
     ChangeStatus Changed = AAAlignFloating::updateImpl(A);
     if (Argument *Arg = getAssociatedArgument()) {

llvm/unittests/IR/FunctionTest.cpp

 }
 
 TEST(FunctionTest, GetPointerAlignment) {
   LLVMContext Context;
   Type *VoidType(Type::getVoidTy(Context));
   FunctionType *FuncType(FunctionType::get(VoidType, false));
   std::unique_ptr<Function> Func(Function::Create(
       FuncType, GlobalValue::ExternalLinkage));
-  EXPECT_EQ(MaybeAlign(), Func->getPointerAlignment(DataLayout("")));
+  EXPECT_EQ(Align(1), Func->getPointerAlignment(DataLayout("")));
   EXPECT_EQ(Align(1), Func->getPointerAlignment(DataLayout("Fi8")));
   EXPECT_EQ(Align(1), Func->getPointerAlignment(DataLayout("Fn8")));
   EXPECT_EQ(Align(2), Func->getPointerAlignment(DataLayout("Fi16")));
   EXPECT_EQ(Align(2), Func->getPointerAlignment(DataLayout("Fn16")));
   EXPECT_EQ(Align(4), Func->getPointerAlignment(DataLayout("Fi32")));
   EXPECT_EQ(Align(4), Func->getPointerAlignment(DataLayout("Fn32")));
 
   Func->setAlignment(Align(4));
 
-  EXPECT_EQ(MaybeAlign(), Func->getPointerAlignment(DataLayout("")));
+  EXPECT_EQ(Align(1), Func->getPointerAlignment(DataLayout("")));
   EXPECT_EQ(Align(1), Func->getPointerAlignment(DataLayout("Fi8")));
   EXPECT_EQ(Align(4), Func->getPointerAlignment(DataLayout("Fn8")));
   EXPECT_EQ(Align(2), Func->getPointerAlignment(DataLayout("Fi16")));
   EXPECT_EQ(Align(4), Func->getPointerAlignment(DataLayout("Fn16")));
   EXPECT_EQ(Align(4), Func->getPointerAlignment(DataLayout("Fi32")));
   EXPECT_EQ(Align(4), Func->getPointerAlignment(DataLayout("Fn32")));
 }
 
 } // end namespace