[PATCH] D63547: [AIX]Global Address Lowering

[Sean Fertile via Phabricator via llvm-commits]

sfertile added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:5070
+    const bool isPPC64 = PPCSubTarget->isPPC64();
+    const bool isELFABI = PPCSubTarget->isTargetELF();
+    const bool isAIXABI = PPCSubTarget->isAIXABI();
----------------
minor nit: we should probably check for SVR4 explicitly instead of if the object file format is ELF, just to be pedantic.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:5074
+
+    assert (!isDarwin && (isELFABI || isAIXABI) && 
+            "Only supported for ELF ABI and AIX ABI");
----------------
nit: just check for `isDarwin`.  Its really just an easy way to convey to the reader that Darwin code never reaches here. `
!isDarwin && (isELFABI || isAIXABI)` is tautological.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:5093
+    // either 32-bit or 64-bit mode.
+    // The first source operand is a TargetGlobalAddress or a TargetJumpTable.
+    auto replaceForMediumOrLargeCM = [this, dl, isPPC64](SDNode *TocEntry) {
----------------
nit: Couldn't the first operand also be a constant pool index or a block address? Maybe we just say its the symbol-reference?


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:5119
+    if (!isPPC64) {
+      if (isELFABI) {
+        const Module *M = MF->getFunction().getParent();
----------------
nit: combine the 2 ifs into a single condition.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:5127
+        return;
+      } else if (isAIXABI && CModel == CodeModel::Small) {
+        replaceWithLWZtoc(N);
----------------
Follows form previous comment: I think this would be more readable if we try to keep the nesting to a minimum, eg:

```
if (!isPPC64 && isELFABI) {
   ....
   return;
}

if (!isPPC64 && isAIX && CModel == CodeModel::Small) {
  ...
return;
}

```


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:5142
+    // handle this, selecting one of LDtoc, LDtocJTI, LDtocCPT, and LDtocBA.
     if (CModel != CodeModel::Medium && CModel != CodeModel::Large)
       break;
----------------
Can we move this up either directly before or directly after the medium code model error for AIX?



================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.td:3173
+                      (PPCtoc_entry tglobaladdr:$disp, i32:$reg))]>;
+def ADDIStocHA32: PPCEmitTimePseudo<(outs gprc:$rD), (ins gprc:$reg, tocentry32:$disp),
+                        "#ADDIStocHA32",
----------------
I think the naming convention is for 32-bit instructions is to have no number appended to the name, and 64-bit instructions have '8' appended.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63547