[PATCH] D70461: [AIX] Emit TOC entries for ASM printing

[Jason Liu via Phabricator via llvm-commits]

jasonliu added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1765
+
+  // Handle external global variables later.
+  if (!GV->hasInitializer()) {
----------------
"Handle ... later" sounds confusing for an early return.
It's more like, we don't need to do anything for external global variables.
And this early return could be just after "ValidateGV(GV);", I think we already set storage class for the symbol in getMCSymbolForTOCPseudoMO. Is that right?



================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1849
+  for (auto &I : TOC) {
+    // Setup the Csect for the current TC entry.
+    MCSectionXCOFF *TCEntry = OutStreamer->getContext().getXCOFFSection(
----------------
nit: Csect -> csect


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1863
+PPCAIXAsmPrinter::getMCSymbolForTOCPseudoMO(const MachineOperand &MO) {
+  const GlobalObject *GO;
+
----------------
Assign a nullptr to it?


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1873
+  // eventually.
+  if (auto *GV = dyn_cast<GlobalVariable>(GO)) {
+    ValidateGV(GV);
----------------
We used *GV again below, we could avoid dyn_cast the second time below if we make it a local variable. 
And could *GV be const?


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1879
+
+  // If the global value is a global variable without initializer or is a
+  // declaration of a function, then MCSymbol XSym represents an external.
----------------
nit: the global value -> the GlobalObject


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1880
+  // If the global value is a global variable without initializer or is a
+  // declaration of a function, then MCSymbol XSym represents an external.
+  // Hence we may need to explictly create a MCSectionXCOFF for it so we can
----------------
nit: "then MCSymbol XSym represents an external" -> "then XSym is an external referenced symbol."


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1882
+  // Hence we may need to explictly create a MCSectionXCOFF for it so we can
+  // return it's symbol later.
+  if (GO->isDeclaration() && !XSym->hasContainingCsect()) {
----------------
nit: it's -> its


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1897
+
+  // Handle inialized globals.
+  if (auto *GV = dyn_cast<GlobalVariable>(GO)) {
----------------
inialized globals-> initialized global variables


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1907
+    }
+
+    return getSymbol(GV);
----------------
We could add a comment here about other global variables, something like:
Other global variables are stored as labels inside of data/text section, we could not refer to them by qualname.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1911
+
+  // If the MO is a function, we want to make sure to refer to the descriptor.
+  return XSym->getContainingCsect()->getQualNameSymbol();
----------------
nit: refer to the descriptor -> refer to the function descriptor csect.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70461