[PATCH] D101178: [XCOFF][AIX] Add Global Variables Directly to TOC for 32 bit AIX

[Sean Fertile via Phabricator via llvm-commits]

sfertile added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:2144
 
+  // If the Global Variable has the toc-data attribute, return
+  if (GV->hasAttribute("toc-data")) {
----------------
I think we need to explain why we return early here. Maybe change `, return`
to 
'it  needs to be emitted later, when we emit the .toc section.'


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:2261
 void PPCAIXAsmPrinter::emitEndOfAsmFile(Module &M) {
   // If there are no functions in this module, we will never need to reference
   // the TOC base.
----------------
Need to update this comment. `if there are no functions in the module, and no toc-data definitions`


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:2283
+
+  for (auto *GV : TOCDataGlobalVars)
+    emitGlobalVariableHelper(GV);
----------------
Please address the clang-tidy warning.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:5545
+      SDValue Op = N->getOperand(0);
+      if (GlobalAddressSDNode *GA = dyn_cast<GlobalAddressSDNode>(Op))
+        if (const GlobalValue *GV = GA->getGlobal())
----------------
Since all we need is true/false for if the global has the toc-data attribute I think we should factor all this casting into an appropriately named helper function. Then we can change the `replaceWithLWZtoc` lambda to take an opcode argument and reuse it for transforming the node.

A helper along the lines of
```
bool hasTocDataAttr(SDValue Val) {
   GlobalAddressSDNode *GA = dyn_cast<GlobalAddressSDNode>(Val);
   if (!GA)
     return false;

   const GlobalVariable *GV = dyn_cast_or_null<GlobalVariable>(GA->getGlobal());
  if (!GV)
    return false;

  if (!GV->hasAttribute("toc-data"))
    return false;


  // TODO assert on alignment and size ...?

  return true;
}
```


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:5566
       // Transforms the ISD::TOC_ENTRY node to a PPCISD::LWZtoc.
       auto replaceWithLWZtoc = [this, &dl](SDNode *TocEntry) {
         SDValue GA = TocEntry->getOperand(0);
----------------
Rename this and add a new parameter `auto replaceWith = [this, &dl](unsigned Opcode, SDNode *TocEntry)`.
Then change the hardcoded LWZtoc to the opcode argument, and move where we do the toc-data transformation down to existing ` if (isAIXABI && CModel == CodeModel::Small)` block on line 5584.


================
Comment at: llvm/test/CodeGen/PowerPC/basic-toc-data-2.ll:12
+; CHECK:        la 3, i[TD](2)
+; CHECK:        .toc                                                                              
+; CHECK-NEXT:   .extern i[TD]
----------------
There is trailing whitespace on this line.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101178