[PATCH] D122198: [PowerPC] Add .ref in backend for AIX XCOFF to support `-bcdtors:csect` linker option

Wed Apr 6 07:22:28 PDT 2022

shchenz added inline comments.

================
Comment at: llvm/include/llvm/MC/MCStreamer.h:625
+  virtual void emitXCOFFRefDirective(StringRef Sym,
+                                     const MCSymbol *Symbol = nullptr);

----------------
Can we only use one parameter `const MCSymbol *Symbol` for this function? And update other callers accordingly. These two parameters are kind of duplicate.

================
Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:2335
+    // Static global variable has associated initializer emits .ref to indicate
+    // the relationship. However .ref does not support BS mapping class.
+    if (Kind.isBSSLocal() &&
----------------
> However .ref does not support BS mapping class.

This seems weird. Should be "csect with BS storage mapping class can not contain .ref"?

================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:2381

   SectionKind GVKind = getObjFileLowering().getKindForGlobal(GV, TM);
   if (!GVKind.isGlobalWriteableData() && !GVKind.isReadOnly() &&
----------------
can we change the `GVkind` here for static globals with associated metadata? We use BSS kind for normal static globals, maybe we can change it to RW, so that we may don't need to change `MCSectionXCOFF::printSwitchToSection` and `TargetLoweringObjectFileXCOFF::SelectSectionForGlobal`?

================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:2407
+      (GVKind.isBSSLocal() &&
+       GV->getMetadata(LLVMContext::MD_associated) == nullptr) ||
       GVKind.isThreadBSSLocal()) {
----------------
nit: `!GV->getMetadata(LLVMContext::MD_associated)` seems more common

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122198