[PATCH] D64652: [AIX] Add a TargetLoweringObjectFile for XCOFF and add support for common variables.

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/TargetLoweringObjectFileImpl.h:223
+
+  void Initialize(MCContext &Ctx, const TargetMachine &TM) override;
+
----------------
Please place these in order:
Initialize
shouldPutJumpTableInFunctionSection
getExplicitSectionGlobal
getStaticCtorSection
getStaticDtorSection
lowerRelativeReference
SelectSectionForGlobal



================
Comment at: llvm/include/llvm/MC/MCSectionXCOFF.h:26
 // as a csect. A csect represents the smallest possible unit of data/code which
-// will be relocated as a single block.
+// will be relocated as a single block. A csect can either be:
+// 1) Initialized: The Type will be XTY_SD, and the symbols inside the csect
----------------
The common should indicate its scope as applying only to the current state of the implementation in LLVM (and not as a general statement on XCOFF).


================
Comment at: llvm/include/llvm/MC/MCSectionXCOFF.h:43
+    assert((ST == XCOFF::XTY_SD || ST == XCOFF::XTY_CM) &&
+           "Unexpected type for csect.");
+  }
----------------
Suggestion: Invalid or unhandled type for csect.


================
Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:1819
+    const GlobalObject *GO, SectionKind Kind, const TargetMachine &TM) const {
+  llvm_unreachable("XCOFF explicit sections not yet implemented.");
+}
----------------
`report_fatal_error`.


================
Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:1841
+    bool UsesLabelDifference, const Function &F) const {
+  llvm_unreachable("TLOF XCOFF not yet implemented.");
+}
----------------
`report_fatal_error`.


================
Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:1854
+    unsigned Priority, const MCSymbol *KeySym) const {
+  llvm_unreachable("XCOFF ctor section not yet implemented.");
+}
----------------
`report_fatal_error`.


================
Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:1859
+    unsigned Priority, const MCSymbol *KeySym) const {
+  llvm_unreachable("XCOFF dtor section not yet implemented.");
+}
----------------
`report_fatal_error`.


================
Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:1865
+    const TargetMachine &TM) const {
+  llvm_unreachable("XCOFF not yet implemented.");
+}
----------------
`report_fatal_error`.


================
Comment at: llvm/lib/MC/MCSectionXCOFF.cpp:23
+    if (getMappingClass() != XCOFF::XMC_PR)
+      llvm_unreachable("Unsupported storage-mapping class for .text csect");
+
----------------
Is the ".text" here meant to be the XCOFF section name? If so, then the comment on line 33 could say ".bss" instead of "common" to be consistent.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1659
+  if (GV->hasComdat())
+    report_fatal_error("COMDAT not yet supported on AIX.");
+
----------------
This is true for the OS and not just LLVM: `s/on/by/`.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1663
+  if (!GVKind.isCommon())
+    report_fatal_error("Only common variables are supported on AIX.");
+
----------------
Add "for now".


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1666
+  // Create the containing csect and switch to it.
+  MCSectionXCOFF *CSect = dyn_cast<MCSectionXCOFF>(
+      getObjFileLowering().SectionForGlobal(GV, GVKind, TM));
----------------
Use `cast` unless if we are expecting the cast to possibly fail and passing NULL to `SwitchSection` is okay.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1671
+  // Create the symbol and emit it.
+  MCSymbolXCOFF *XSym = dyn_cast<MCSymbolXCOFF>(getSymbol(GV));
+  auto DL = GV->getParent()->getDataLayout();
----------------
See comment above.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1672
+  MCSymbolXCOFF *XSym = dyn_cast<MCSymbolXCOFF>(getSymbol(GV));
+  auto DL = GV->getParent()->getDataLayout();
+  unsigned Align =
----------------
`llvm::DataLayout` has a non-trivial destructor. I advise against copying.


================
Comment at: llvm/test/CodeGen/PowerPC/aix-xcoff-common.ll:9
+ at f = common local_unnamed_addr global float 0.000000e+00, align 4
+ at comm = common local_unnamed_addr global double 0.000000e+00, align 8
+
----------------
What is the intended difference between "d" and "comm"?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64652