[PATCH] D68341: [AIX] TOC pseudo expansion for 64bit large + 64bit small + 32bit large modes

[Sean Fertile via Phabricator via llvm-commits]

sfertile added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:752
+    const MCSymbolRefExpr::VariantKind VK =
+        !IsAIX ? MCSymbolRefExpr::VK_PPC_TOC : MCSymbolRefExpr::VK_None;
     const MCExpr *Exp =
----------------
Very minor nit: We can slightly simplify if you drop the `!' and reverse the true/false sides.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:826
+    // Change the opcode to ADDIS8.  If the global address is the address of
+    // an external symbol, is a jump table address, is a block address; or if
+    // large code model is enabled then generate a TOC entry and reference that.
----------------
Was the ';' after 'block-address' meant to be a comma?


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:837
 
+    bool GlobalToc = false;
     if (MO.isGlobal()) {
----------------
Minor nit: We can fold away the if:

`bool GlobalToc = MO.isGlobal() && Subtarget->isGVIndirectSymbol(MO.getGlobal());`


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:879
+
+    if (MO.isGlobal()) {
       LLVM_DEBUG(
----------------
Instead of using an 'if' statement that will be empty when assertions are disabled, can we fold this condition into the assert? ie
```
LLVM_DEBUG(
  assert((!MO.isGlobal() || Subtarget->isGVIndirectSymbol(MO.getGlobal())) && ...
```


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:912
     if (MO.isGlobal()) {
-      const GlobalValue *GV = MO.getGlobal();
-      LLVM_DEBUG(assert(!(Subtarget->isGVIndirectSymbol(GV)) &&
+      LLVM_DEBUG(assert(!(Subtarget->isGVIndirectSymbol(MO.getGlobal())) &&
                         "Interposable definitions must use indirect access."));
----------------
Ditto on folding away the if.


================
Comment at: llvm/test/CodeGen/PowerPC/lower-globaladdr32-aix-asm.ll:2
 ; RUN: llc -mtriple powerpc-ibm-aix-xcoff \
-; RUN: -code-model=small < %s | FileCheck %s
+; RUN: -code-model=small < %s | FileCheck %s --check-prefix=SMALL
+
----------------
Please add '-verify-machine-instr` and an mcpu  option to each llc invocation. 


================
Comment at: llvm/test/CodeGen/PowerPC/lower-globaladdr64-aix-asm.ll:2
+; RUN: llc -mtriple powerpc64-ibm-aix-xcoff \
+; RUN: -code-model=small < %s | FileCheck %s --check-prefix=SMALL
+
----------------
ditto.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68341