[PATCH] D142708: [NFC] Transition GlobalObject alignment from MaybeAlign to Align

[Guillaume Chatelet via Phabricator via llvm-commits]

gchatelet marked an inline comment as done.
gchatelet added a comment.

In D142708#4086420 <https://reviews.llvm.org/D142708#4086420>, @efriedma wrote:

> In D142708#4085789 <https://reviews.llvm.org/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 <https://github.com/llvm/llvm-project/blob/72121a20cda4dc91d0ef5548f93046e71c5ec6f6/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp#L2833>, 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);



================
Comment at: llvm/include/llvm/IR/GlobalObject.h:86
+  /// Sets the alignment attribute of the GlobalObject.
+  void setAlignment(Align Align);
+
----------------
efriedma wrote:
> Not sure I understand the point of overloading setAlignment; there's an implicit conversion from Align to MaybeAlign.
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`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142708/new/

https://reviews.llvm.org/D142708