[NFC] Transition GlobalObject alignment from MaybeAlign to Align

[gchatelet]

Summary

This is a follow up on https://reviews.llvm.org/D142459#4081179.
This first patch adds an overload to GlobalObject::setAlignment that accepts an Align type.
This already handles most of the calls.

This patch also converts a few call sites to the new type when this is safe.

Here is the list of the remaining call sites:

• clang/lib/CodeGen/CodeGenModule.cpp:1688
• llvm/lib/AsmParser/LLParser.cpp:1309
• llvm/lib/AsmParser/LLParser.cpp:6050
• llvm/lib/Bitcode/Reader/BitcodeReader.cpp:3871
• llvm/lib/Bitcode/Reader/BitcodeReader.cpp:4030
• llvm/lib/IR/Core.cpp:2018
• llvm/lib/IR/Globals.cpp:141
• llvm/lib/Linker/IRMover.cpp:660
• llvm/lib/Linker/LinkModules.cpp:361
• llvm/lib/Linker/LinkModules.cpp:362
• llvm/lib/Transforms/IPO/MergeFunctions.cpp:782
• llvm/lib/Transforms/IPO/MergeFunctions.cpp:840
• llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:1813
• llvm/tools/llvm-reduce/deltas/ReduceGlobalObjects.cpp:27

[courbet]

This one is NFC, tag it as such ?

llvm/include/llvm/IR/GlobalObject.h

90

defined*

[gchatelet]

• rebase, fix typo and amend commit message to add [NFC]

[gchatelet]

@efriedma regarding the remaining calls, some of them will autosolve when getAlign will return Align instead of MaybeAlign but there are a few ones for which I'm unsure how to proceed.
More particularly the following ones that deal with creating new GlobalObjects. Is this reasonable to align to 1 when unknown or can we do better?

• llvm/lib/AsmParser/LLParser.cpp:1309
• llvm/lib/AsmParser/LLParser.cpp:6050
• llvm/lib/Bitcode/Reader/BitcodeReader.cpp:3871
• llvm/lib/Bitcode/Reader/BitcodeReader.cpp:4030

[nikic]

@gchatelet I believe you'll want to use the preferred type alignment. Looking at https://github.com/llvm/llvm-project/blob/72121a20cda4dc91d0ef5548f93046e71c5ec6f6/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp#L324, we will currently pick the preferred type alignment as the default.

[gchatelet]

In D142708#4085604, @nikic wrote:

@gchatelet I believe you'll want to use the preferred type alignment. Looking at https://github.com/llvm/llvm-project/blob/72121a20cda4dc91d0ef5548f93046e71c5ec6f6/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp#L324, we will currently pick the preferred type alignment as the default.

Thx for the suggestion @nikic!
If I manually apply dead code elimination to the code you're referring to with GV being a Function and GV->getAlign() returning std::nullopt I have the following code

Align AsmPrinter::getGVAlignment(const GlobalObject *GV, const DataLayout &DL, Align InAlign = Align(1)) {
  Align Alignment;
  if (InAlign > Alignment)
    Alignment = InAlign;
  return Alignment;
}

Which is basically std::max(InAlign, Align(1)) or simply Align(1) as I don't see an obvious value for InAlign other than 1.

So 1 it is?

[gchatelet]

fix formatting

[efriedma]

In D142708#4085789, @gchatelet wrote:

Which is basically std::max(InAlign, Align(1)) or simply Align(1) as I don't see an obvious value for InAlign other than 1.

Some callers pass in other values.

llvm/include/llvm/IR/GlobalObject.h

86

Not sure I understand the point of overloading setAlignment; there's an implicit conversion from Align to MaybeAlign.

[gchatelet]

In D142708#4086420, @efriedma wrote:

In D142708#4085789, @gchatelet wrote:

Which is basically std::max(InAlign, Align(1)) or simply Align(1) as I don't see an obvious value for InAlign other than 1.

Some callers pass in other values.

Yes indeed, the only call to this function specifying the InAlign argument is this one, appearing in AsmPrinter::emitAlignment.
Unfortunately this one is called a lot (32 instances). There are some arch specific tunings as well :-/

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:810:      emitAlignment(Alignment, GV);
llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:846:  emitAlignment(Alignment, GV);
llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:903:    emitAlignment(MF->getAlignment(), &F);
llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:2145:      emitAlignment(Align(DL.getPointerSize()));
llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:2172:        emitAlignment(Align(DL.getPointerSize()));
llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:2452:        emitAlignment(Align(CPSections[i].Alignment));
llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:2497:  emitAlignment(Align(MJTI->getEntryAlignment(DL)));
llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:2732:      emitAlignment(Align);
llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:2830:void AsmPrinter::emitAlignment(Align Alignment, const GlobalObject *GV,
llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:3691:    emitAlignment(Alignment, nullptr, MBB.getMaxBytesForAlignment());
llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:4021:    emitAlignment(Align(PointerSize));
llvm/lib/CodeGen/AsmPrinter/EHStreamer.cpp:473:  Asm->emitAlignment(Align(4));
llvm/lib/CodeGen/AsmPrinter/EHStreamer.cpp:673:        Asm->emitAlignment(Align(4));
llvm/lib/CodeGen/AsmPrinter/EHStreamer.cpp:802:    Asm->emitAlignment(Align(4));
llvm/lib/CodeGen/AsmPrinter/EHStreamer.cpp:806:  Asm->emitAlignment(Align(4));
llvm/lib/CodeGen/AsmPrinter/ErlangGCPrinter.cpp:72:    AP.emitAlignment(IntPtrSize == 4 ? Align(4) : Align(8));
llvm/lib/CodeGen/AsmPrinter/OcamlGCPrinter.cpp:128:  AP.emitAlignment(IntPtrSize == 4 ? Align(4) : Align(8));
llvm/lib/CodeGen/AsmPrinter/OcamlGCPrinter.cpp:179:      AP.emitAlignment(IntPtrSize == 4 ? Align(4) : Align(8));
llvm/lib/CodeGen/AsmPrinter/WinException.cpp:208:    Asm->emitAlignment(std::max(Asm->MF->getAlignment(), MBB.getAlignment()),
llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp:946:    emitAlignment(Align(Size));
llvm/lib/Target/ARM/ARMAsmPrinter.cpp:179:    emitAlignment(Align(2));
llvm/lib/Target/ARM/ARMAsmPrinter.cpp:537:      emitAlignment(Align(4));
llvm/lib/Target/ARM/ARMAsmPrinter.cpp:550:      emitAlignment(Align(4));
llvm/lib/Target/ARM/ARMAsmPrinter.cpp:983:  emitAlignment(Align(4));
llvm/lib/Target/ARM/ARMAsmPrinter.cpp:1029:  emitAlignment(Align(4));
llvm/lib/Target/ARM/ARMAsmPrinter.cpp:1058:    emitAlignment(Align(4));
llvm/lib/Target/ARM/ARMAsmPrinter.cpp:1101:  emitAlignment(Align(2));
llvm/lib/Target/Mips/MipsAsmPrinter.cpp:406:    emitAlignment(std::max(MF->getAlignment(), MIPS_NACL_BUNDLE_ALIGN));
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:2443:  emitAlignment(getGVAlignment(GV, DL), GV);
llvm/lib/Target/X86/X86AsmPrinter.cpp:774:      emitAlignment(WordSize == 4 ? Align(4) : Align(8));
llvm/lib/Target/X86/X86AsmPrinter.cpp:784:      emitAlignment(WordSize == 4 ? Align(4) : Align(8)); // padding
llvm/lib/Target/XCore/XCoreAsmPrinter.cpp:145:  emitAlignment(std::max(Alignment, Align(4)), GV);

llvm/include/llvm/IR/GlobalObject.h

86

The purpose here is to define a deprecation path for downstream users.
Once we've cleaned up calls in the LLVM repo we will mark the void setAlignment(MaybeAlign Align); with LLVM_DEPRECATED to help downstream users migrate to the new API.
It also helps catch all call sites by marking the function deprecated and compile the codebase with -DLLVM_ENABLE_WERROR=ON

[gchatelet]

I'm submitting this now as it is NFC.
I'll follow up on a case by case basis.

[gchatelet]

Breakage came from GCC being more strict on type redefinition

/b/sanitizer-x86_64-linux/build/llvm-project/llvm/include/llvm/LTO/LTO.h:292:13: error: declaration of ‘llvm::Align llvm::lto::LTO::RegularLTOState::CommonResolution::Align’ changes meaning of ‘Align’ [-fpermissive]
  292 |       Align Align;
      |             ^~~~~

I changed the variable name to Alignment instead.

llvm/include/llvm/IR/GlobalObject.h

public:
   /// Note that for functions this is the alignment of the code, not the
   /// alignment of a function pointer.
   MaybeAlign getAlign() const {
     unsigned Data = getGlobalValueSubClassData();
     unsigned AlignmentData = Data & AlignmentMask;
     return decodeMaybeAlign(AlignmentData);
   }
 
+  /// Sets the alignment attribute of the GlobalObject.
+  void setAlignment(Align Align);
+
+  /// Sets the alignment attribute of the GlobalObject.
+  /// This method will be deprecated as the alignment property should always be
+  /// defined.
   void setAlignment(MaybeAlign Align);
 
   unsigned getGlobalObjectSubClassData() const {
     unsigned ValueData = getGlobalValueSubClassData();
     return ValueData >> GlobalObjectBits;
   }
 
   void setGlobalObjectSubClassData(unsigned Val) {

llvm/include/llvm/LTO/LTO.h

 private:
   Config Conf;
 
   struct RegularLTOState {
     RegularLTOState(unsigned ParallelCodeGenParallelismLevel,
                     const Config &Conf);
     struct CommonResolution {
       uint64_t Size = 0;
-      MaybeAlign Align;
+      Align Align;
       /// Record if at least one instance of the common was marked as prevailing
       bool Prevailing = false;
     };
     std::map<std::string, CommonResolution> Commons;
 
     unsigned ParallelCodeGenParallelismLevel;
     LTOLLVMContext Ctx;
     std::unique_ptr<Module> CombinedModule;

llvm/lib/AsmParser/LLParser.cpp

if (Function *Fn = dyn_cast<Function>(V)) {
       AttrBuilder FnAttrs(M->getContext(), AS.getFnAttrs());
       AS = AS.removeFnAttributes(Context);
 
       FnAttrs.merge(B);
 
       // If the alignment was parsed as an attribute, move to the alignment
       // field.
       if (MaybeAlign A = FnAttrs.getAlignment()) {
-        Fn->setAlignment(A);
+        Fn->setAlignment(*A);
         FnAttrs.removeAttribute(Attribute::Alignment);
       }
 
       AS = AS.addFnAttributes(Context, FnAttrs);
       Fn->setAttributes(AS);
     } else if (CallInst *CI = dyn_cast<CallInst>(V)) {
       AttributeList AS = CI->getAttributes();
       AttrBuilder FnAttrs(M->getContext(), AS.getFnAttrs());
bool LLParser::parseFunctionHeader(Function *&Fn, bool IsDefine) {
 
   Fn->setLinkage((GlobalValue::LinkageTypes)Linkage);
   maybeSetDSOLocal(DSOLocal, *Fn);
   Fn->setVisibility((GlobalValue::VisibilityTypes)Visibility);
   Fn->setDLLStorageClass((GlobalValue::DLLStorageClassTypes)DLLStorageClass);
   Fn->setCallingConv(CC);
   Fn->setAttributes(PAL);
   Fn->setUnnamedAddr(UnnamedAddr);
-  Fn->setAlignment(MaybeAlign(Alignment));
+  Fn->setAlignment(Alignment);
   Fn->setSection(Section);
   Fn->setPartition(Partition);
   Fn->setComdat(C);
   Fn->setPersonalityFn(PersonalityFn);
   if (!GC.empty()) Fn->setGC(GC);
   Fn->setPrefixData(Prefix);
   Fn->setPrologueData(Prologue);
   ForwardRefAttrGroups[Fn] = FwdRefAttrGrps;

llvm/lib/IR/Globals.cpp

void GlobalObject::setAlignment(MaybeAlign Align) {
   assert((!Align || *Align <= MaximumAlignment) &&
          "Alignment is greater than MaximumAlignment!");
   unsigned AlignmentData = encode(Align);
   unsigned OldData = getGlobalValueSubClassData();
   setGlobalValueSubClassData((OldData & ~AlignmentMask) | AlignmentData);
   assert(getAlign() == Align && "Alignment representation error!");
 }
 
+void GlobalObject::setAlignment(Align Align) {
+  assert(Align <= MaximumAlignment &&
+         "Alignment is greater than MaximumAlignment!");
+  unsigned AlignmentData = encode(Align);
+  unsigned OldData = getGlobalValueSubClassData();
+  setGlobalValueSubClassData((OldData & ~AlignmentMask) | AlignmentData);
+  assert(getAlign() && *getAlign() == Align &&
+         "Alignment representation error!");
+}
+
 void GlobalObject::copyAttributesFrom(const GlobalObject *Src) {
   GlobalValue::copyAttributesFrom(Src);
   setAlignment(Src->getAlign());
   setSection(Src->getSection());
 }
 
 std::string GlobalValue::getGlobalIdentifier(StringRef Name,
                                              GlobalValue::LinkageTypes Linkage,

llvm/lib/LTO/LTO.cpp

for (const InputFile::Symbol &Sym : Syms) {
     // We also record if we see an instance of a common as prevailing, so that
     // if none is prevailing we can ignore it later.
     if (Sym.isCommon()) {
       // FIXME: We should figure out what to do about commons defined by asm.
       // For now they aren't reported correctly by ModuleSymbolTable.
       auto &CommonRes = RegularLTO.Commons[std::string(Sym.getIRName())];
       CommonRes.Size = std::max(CommonRes.Size, Sym.getCommonSize());
       if (uint32_t SymAlignValue = Sym.getCommonAlignment()) {
-        const Align SymAlign(SymAlignValue);
-        CommonRes.Align = std::max(SymAlign, CommonRes.Align.valueOrOne());
+        CommonRes.Align = std::max(Align(SymAlignValue), CommonRes.Align);
       }
       CommonRes.Prevailing |= Res.Prevailing;
     }
   }
 
   if (!M.getComdatSymbolTable().empty())
     for (GlobalValue &GV : M.global_values())
       handleNonPrevailingComdat(GV, NonPrevailingComdats);

llvm/lib/Transforms/IPO/OpenMPOpt.cpp

for (CallBase *CB : MallocCalls) {
                   << ((AllocSize->getZExtValue() != 1) ? " bytes " : " byte ")
                   << "of shared memory.";
       };
       A.emitRemark<OptimizationRemark>(CB, "OMP111", Remark);
 
       MaybeAlign Alignment = CB->getRetAlign();
       assert(Alignment &&
              "HeapToShared on allocation without alignment attribute");
-      SharedMem->setAlignment(MaybeAlign(Alignment));
+      SharedMem->setAlignment(*Alignment);
 
       A.changeAfterManifest(IRPosition::callsite_returned(*CB), *NewBuffer);
       A.deleteAfterManifest(*CB);
       A.deleteAfterManifest(*FreeCalls.front());
 
       SharedMemoryUsed += AllocSize->getZExtValue();
       NumBytesMovedToSharedMemory = SharedMemoryUsed;
       Changed = ChangeStatus::CHANGED;

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp

for (size_t i = 0; i < n; i++) {
     GlobalValue::LinkageTypes Linkage = G->getLinkage();
     if (G->isConstant() && Linkage == GlobalValue::PrivateLinkage)
       Linkage = GlobalValue::InternalLinkage;
     GlobalVariable *NewGlobal = new GlobalVariable(
         M, NewTy, G->isConstant(), Linkage, NewInitializer, "", G,
         G->getThreadLocalMode(), G->getAddressSpace());
     NewGlobal->copyAttributesFrom(G);
     NewGlobal->setComdat(G->getComdat());
-    NewGlobal->setAlignment(MaybeAlign(getMinRedzoneSizeForGlobal()));
+    NewGlobal->setAlignment(Align(getMinRedzoneSizeForGlobal()));
     // Don't fold globals with redzones. ODR violation detector and redzone
     // poisoning implicitly creates a dependence on the global's address, so it
     // is no longer valid for it to be marked unnamed_addr.
     NewGlobal->setUnnamedAddr(GlobalValue::UnnamedAddr::None);
 
     // Move null-terminated C strings to "__asan_cstring" section on Darwin.
     if (TargetTriple.isOSBinFormatMachO() && !G->hasSection() &&
         G->isConstant()) {