[clang] Deprecate uses of GlobalObject::getAlignment

[gchatelet]

Summary

This remove the last uses of GlobalObject::getAlignment and marks it as deprecated.

[courbet]

clang/include/clang/AST/CharUnits.h

73

Maybe*

clang/lib/CodeGen/CGObjCMac.cpp

1991

I'm wondering whetehr we could just do away with this conversion and store a MaybeAlign in GlobalValue instead.

The only uses of Address::getAlign() is to creata another Address or do Address::getAlign()->getAsAlign()

[tra]

clang/lib/CodeGen/CGCUDANV.cpp

491

This appears to be a change in behavior. AFAICT, previously used Var->getAlignment() could return alignment value or 0. Now it's value or 1.

Is it intentional?

[gchatelet]

clang/lib/CodeGen/CGCUDANV.cpp

491

The previous statement was constructing an llvm::Align from a value, and llvm::Align asserts when the provided value is 0. This means that it is undefined to pass the value 0.

As far as LoadInst is concerned it can only accept valid alignments (i.e., powers of two => non zero).

So you're right that it is not strictly NFC and that *Var->getAlign()would be a more rigorous transformation but I think that converting the 0 value to 1 moves us from UB to semantically valid code.

I don't feel strongly about it though and I'm fine changing this to *Var->getAlign() to keep this patch NFC. WDYT?

[tra]

clang/lib/CodeGen/CGCUDANV.cpp

491

Enforcing alignment of 1 would potentially force us to generate overly conservative one byte at a time loads/stores.
I agree that passing 0 is a wrong choice here, but 1 does not seem to be correct, either.
Unfortunately LoadInst does not have overloads accepting MaybeAlign so we need to use different LoadInst overload depending on whether alignment is specified.

NewV =  Var->getAlign().isAligned() 
  ? llvm::LoadInst(Var->getType(), ManagedVar, "ld.managed", false,  Var->getAlign().value(), I)
  : llvm::LoadInst(Var->getType(), ManagedVar, "ld.managed", false,  I);

@yaxunl -- Sam, does it make sense? This seems to be largely HIP-specific.

[barannikov88]

clang/lib/CodeGen/CGCUDANV.cpp

491

I think it should be just Var->getAlign() to allow propagating absent alignment.
Curiously, the old code didn't assert because GlobalVariable seem to always(?) have non-empty alignment if the global had an associated VarDecl (set here, changed(?) by this patch).

[barannikov88]

clang/lib/CodeGen/CGCUDANV.cpp

491

Unfortunately LoadInst does not have overloads accepting MaybeAlign so we need to use different LoadInst overload depending on whether alignment is specified.

That's interesting, because SelectionDAG::getLoad has many variants with MaybeAlign, but only one with Align.

[gchatelet]

clang/lib/CodeGen/CGCUDANV.cpp

491

The documentation says that LoadInst alignment is optional so indeed it should accept a MaybeAlign instead of an Align.
I'll send a patch to fix this.

Then we can indeed simply use Var->getAlign().

[efriedma]

clang/lib/CodeGen/CGCUDANV.cpp

491

Alignment is "optional" in textual IR for backward-compatibility/ease of use... but it's just a shortcut for specifying "use the ABI alignment of the value type". It isn't optional in memory. Maybe LangRef could be updated to make that a bit more clear. (See D77454.)

We should consider doing something similar for globals, but at the time, I ran out of energy to try to deal with that; backend handling for globals with unspecified alignment is weird.

[yaxunl]

clang/lib/CodeGen/CGCUDANV.cpp

491

Enforcing alignment of 1 would potentially force us to generate overly conservative one byte at a time loads/stores.
I agree that passing 0 is a wrong choice here, but 1 does not seem to be correct, either.
Unfortunately LoadInst does not have overloads accepting MaybeAlign so we need to use different LoadInst overload depending on whether alignment is specified.

NewV =  Var->getAlign().isAligned() 
  ? llvm::LoadInst(Var->getType(), ManagedVar, "ld.managed", false,  Var->getAlign().value(), I)
  : llvm::LoadInst(Var->getType(), ManagedVar, "ld.managed", false,  I);

@yaxunl -- Sam, does it make sense? This seems to be largely HIP-specific.

Thanks for reminding me about this.

If it is 0 it should have caused an assertion.

We can probably use getAlign().value() to keep the original asserting behaviour instead of silently using value 1.

[arsenm]

clang/lib/CodeGen/CGCUDANV.cpp

491

I think MaybeAlign is ridiculous and we should just require alignments everywhere. It's never really optional, it's just hidden sometimes

[gchatelet]

Thx @efriedma for reminding me of this.
I'll update the documentation and try to give a shot at removing MaybeAlign from GlobalObject.

clang/include/clang/AST/CharUnits.h

public:
       }
 
       /// fromQuantity - Construct a CharUnits quantity from an llvm::Align
       /// quantity.
       static CharUnits fromQuantity(llvm::Align Quantity) {
         return CharUnits(Quantity.value());
       }
 
+      /// fromQuantity - Construct a CharUnits quantity from an llvm::Align
+      /// quantity.
+      static CharUnits fromQuantity(llvm::MaybeAlign Quantity) {
+        return Quantity ? fromQuantity(*Quantity) : Zero();
+      }
+
       // Compound assignment.
       CharUnits& operator+= (const CharUnits &Other) {
         Quantity += Other.Quantity;
         return *this;
       }
       CharUnits& operator++ () {
         ++Quantity;
         return *this;

clang/lib/CodeGen/CGCUDANV.cpp

if (isa<llvm::ConstantExpr>(U)) {
         WorkItem.pop_back();
       }
       continue;
     }
     if (auto *I = dyn_cast<llvm::Instruction>(U)) {
       llvm::Value *OldV = Var;
       llvm::Instruction *NewV =
           new llvm::LoadInst(Var->getType(), ManagedVar, "ld.managed", false,
-                             llvm::Align(Var->getAlignment()), I);
+                             Var->getAlign().valueOrOne(), I);
       WorkItem.pop_back();
       // Replace constant expressions directly or indirectly using the managed
       // variable with instructions.
       for (auto &&Op : WorkItem) {
         auto *CE = cast<llvm::ConstantExpr>(Op);
         auto *NewInst = CE->getAsInstruction(I);
         NewInst->replaceUsesOfWith(OldV, NewV);
         OldV = CE;
case DeviceVarFlags::Variable: {
         Var->setName(Twine(ManagedVar->getName() + ".managed"));
         replaceManagedVar(Var, ManagedVar);
         llvm::Value *Args[] = {
             &GpuBinaryHandlePtr,
             Builder.CreateBitCast(ManagedVar, VoidPtrTy),
             Builder.CreateBitCast(Var, VoidPtrTy),
             VarName,
             llvm::ConstantInt::get(VarSizeTy, VarSize),
-            llvm::ConstantInt::get(IntTy, Var->getAlignment())};
+            llvm::ConstantInt::get(IntTy, Var->getAlign()->value())};
         if (!Var->isDeclaration())
           Builder.CreateCall(RegisterManagedVar, Args);
       } else {
         llvm::Value *Args[] = {
             &GpuBinaryHandlePtr,
             Builder.CreateBitCast(Var, VoidPtrTy),
             VarName,
             VarName,
GpuBinaryHandle = new llvm::GlobalVariable(
         "__hip_gpubin_handle");
     if (Linkage == llvm::GlobalValue::LinkOnceAnyLinkage)
       GpuBinaryHandle->setComdat(
           CGM.getModule().getOrInsertComdat(GpuBinaryHandle->getName()));
     GpuBinaryHandle->setAlignment(CGM.getPointerAlign().getAsAlign());
     // Prevent the weak symbol in different shared libraries being merged.
     if (Linkage != llvm::GlobalValue::InternalLinkage)
       GpuBinaryHandle->setVisibility(llvm::GlobalValue::HiddenVisibility);
-    Address GpuBinaryAddr(
-        GpuBinaryHandle, VoidPtrPtrTy,
-        CharUnits::fromQuantity(GpuBinaryHandle->getAlignment()));
+    Address GpuBinaryAddr(GpuBinaryHandle, VoidPtrPtrTy,
+                          CharUnits::fromQuantity(GpuBinaryHandle->getAlign()));
     {
       auto *HandleValue = CtorBuilder.CreateLoad(GpuBinaryAddr);
       llvm::Constant *Zero =
           llvm::Constant::getNullValue(HandleValue->getType());
       llvm::Value *EQZero = CtorBuilder.CreateICmpEQ(HandleValue, Zero);
       CtorBuilder.CreateCondBr(EQZero, IfBlock, ExitBlock);
     }
     {
llvm::Function *ModuleDtorFunc = llvm::Function::Create(
       llvm::GlobalValue::InternalLinkage,
       addUnderscoredPrefixToName("_module_dtor"), &TheModule);
 
   llvm::BasicBlock *DtorEntryBB =
       llvm::BasicBlock::Create(Context, "entry", ModuleDtorFunc);
   CGBuilderTy DtorBuilder(CGM, Context);
   DtorBuilder.SetInsertPoint(DtorEntryBB);
 
-  Address GpuBinaryAddr(
-      GpuBinaryHandle, GpuBinaryHandle->getValueType(),
-      CharUnits::fromQuantity(GpuBinaryHandle->getAlignment()));
+  Address GpuBinaryAddr(GpuBinaryHandle, GpuBinaryHandle->getValueType(),
+                        CharUnits::fromQuantity(GpuBinaryHandle->getAlign()));
   auto *HandleValue = DtorBuilder.CreateLoad(GpuBinaryAddr);
   // There is only one HIP fat binary per linked module, however there are
   // multiple destructor functions. Make sure the fat binary is unregistered
   // only once.
   if (CGM.getLangOpts().HIP) {
     llvm::BasicBlock *IfBlock =
         llvm::BasicBlock::Create(Context, "if", ModuleDtorFunc);
     llvm::BasicBlock *ExitBlock =

clang/lib/CodeGen/CGDecl.cpp

if (!CacheEntry || CacheEntry->getInitializer() != Constant) {
     else
       llvm_unreachable("local variable has no parent function or method");
     llvm::GlobalVariable *GV = new llvm::GlobalVariable(
         getModule(), Ty, isConstant, llvm::GlobalValue::PrivateLinkage,
         Constant, Name, InsertBefore, llvm::GlobalValue::NotThreadLocal, AS);
     GV->setAlignment(Align.getAsAlign());
     GV->setUnnamedAddr(llvm::GlobalValue::UnnamedAddr::Global);
     CacheEntry = GV;
-  } else if (CacheEntry->getAlignment() < uint64_t(Align.getQuantity())) {
+  } else if (CacheEntry->getAlign() < uint64_t(Align.getQuantity())) {
     CacheEntry->setAlignment(Align.getAsAlign());
   }
 
   return Address(CacheEntry, CacheEntry->getValueType(), Align);
 }
 
 static Address createUnnamedGlobalForMemcpyFrom(CodeGenModule &CGM,
                                                 const VarDecl &D,

clang/lib/CodeGen/CGObjCMac.cpp

 
 ConstantAddress
 CGObjCCommonMac::GenerateConstantNSString(const StringLiteral *Literal) {
   unsigned StringLength = 0;
   llvm::StringMapEntry<llvm::GlobalVariable *> &Entry =
     GetConstantStringEntry(NSConstantStringMap, Literal, StringLength);
 
   if (auto *C = Entry.second)
-    return ConstantAddress(
-        C, C->getValueType(), CharUnits::fromQuantity(C->getAlignment()));
+    return ConstantAddress(C, C->getValueType(),
+                           CharUnits::fromQuantity(C->getAlign()));
 
   // If we don't already have it, get _NSConstantStringClassReference.
   llvm::Constant *Class = getNSConstantStringClassRef();
 
   // If we don't already have it, construct the type for a constant NSString.
   if (!NSConstantStringType) {
     NSConstantStringType =
       llvm::StructType::create({

clang/lib/CodeGen/CodeGenModule.cpp

if (!D->hasAttr<AlignedAttr>())
     if (LangOpts.FunctionAlignment)
       F->setAlignment(llvm::Align(1ull << LangOpts.FunctionAlignment));
 
   // Some C++ ABIs require 2-byte alignment for member functions, in order to
   // reserve a bit for differentiating between virtual and non-virtual member
   // functions. If the current target's C++ ABI requires this and this is a
   // member function, set its alignment accordingly.
   if (getTarget().getCXXABI().areMemberFunctionsAligned()) {
-    if (F->getAlignment() < 2 && isa<CXXMethodDecl>(D))
+    if (F->getAlign() < 2 && isa<CXXMethodDecl>(D))
       F->setAlignment(llvm::Align(2));
   }
 
   // In the cross-dso CFI mode with canonical jump tables, we want !type
   // attributes on definitions only.
   if (CodeGenOpts.SanitizeCfiCrossDso &&
       CodeGenOpts.SanitizeCfiCanonicalJumpTables) {
     if (auto *FD = dyn_cast<FunctionDecl>(D)) {
CodeGenModule::GetAddrOfConstantCFString(const StringLiteral *Literal) {
   unsigned StringLength = 0;
   bool isUTF16 = false;
   llvm::StringMapEntry<llvm::GlobalVariable *> &Entry =
       GetConstantCFStringEntry(CFConstantStringMap, Literal,
                                getDataLayout().isLittleEndian(), isUTF16,
                                StringLength);
 
   if (auto *C = Entry.second)
-    return ConstantAddress(
-        C, C->getValueType(), CharUnits::fromQuantity(C->getAlignment()));
+    return ConstantAddress(C, C->getValueType(),
+                           CharUnits::fromQuantity(C->getAlign()));
 
   llvm::Constant *Zero = llvm::Constant::getNullValue(Int32Ty);
   llvm::Constant *Zeros[] = { Zero, Zero };
 
   const ASTContext &Context = getContext();
   const llvm::Triple &Triple = getTriple();
 
   const auto CFRuntime = getLangOpts().CFRuntime;
CodeGenModule::GetAddrOfConstantStringFromLiteral(const StringLiteral *S,
                                                   StringRef Name) {
   CharUnits Alignment = getContext().getAlignOfGlobalVarInChars(S->getType());
 
   llvm::Constant *C = GetConstantArrayFromStringLiteral(S);
   llvm::GlobalVariable **Entry = nullptr;
   if (!LangOpts.WritableStrings) {
     Entry = &ConstantStringMap[C];
     if (auto GV = *Entry) {
-      if (uint64_t(Alignment.getQuantity()) > GV->getAlignment())
+      if (GV->getAlign() < uint64_t(Alignment.getQuantity()))
         GV->setAlignment(Alignment.getAsAlign());
       return ConstantAddress(castStringLiteralToDefaultAddressSpace(*this, GV),
                              GV->getValueType(), Alignment);
     }
   }
 
   SmallString<256> MangledNameBuffer;
   StringRef GlobalVariableName;
ConstantAddress CodeGenModule::GetAddrOfConstantCString(
   llvm::Constant *C =
       llvm::ConstantDataArray::getString(getLLVMContext(), StrWithNull, false);
 
   // Don't share any string literals if strings aren't constant.
   llvm::GlobalVariable **Entry = nullptr;
   if (!LangOpts.WritableStrings) {
     Entry = &ConstantStringMap[C];
     if (auto GV = *Entry) {
-      if (uint64_t(Alignment.getQuantity()) > GV->getAlignment())
+      if (GV->getAlign() < uint64_t(Alignment.getQuantity()))
         GV->setAlignment(Alignment.getAsAlign());
       return ConstantAddress(castStringLiteralToDefaultAddressSpace(*this, GV),
                              GV->getValueType(), Alignment);
     }
   }
 
   // Get the default prefix if a name wasn't specified.
   if (!GlobalName)

llvm/include/llvm/IR/GlobalObject.h

private:
   static const unsigned AlignmentBits = LastAlignmentBit + 1;
   static const unsigned AlignmentMask = (1 << AlignmentBits) - 1;
   static const unsigned GlobalObjectMask = (1 << GlobalObjectBits) - 1;
 
 public:
   GlobalObject(const GlobalObject &) = delete;
 
   /// FIXME: Remove this function once transition to Align is over.
+  LLVM_DEPRECATED("Use getAlign() instead", "getAlign")
   uint64_t getAlignment() const {
     MaybeAlign Align = getAlign();
     return Align ? Align->value() : 0;
   }
 
   /// Returns the alignment of the given variable or function.
   ///
   /// Note that for functions this is the alignment of the code, not the