[PATCH] D63547: [AIX]Global Address Lowering

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:5084
+    // For 64-bit small code model, we allow SelectCodeCommon to handle this,
+    // selecting one of LDtoc, LDtocJTI,LDtocCPT, and LDtocBA.
+    if (isPPC64 && CModel == CodeModel::Small)
----------------
Missing space before `LDtocCPT`.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:5088
 
-    // The first source operand is a TargetGlobalAddress or a TargetJumpTable.
-    // If it must be toc-referenced according to PPCSubTarget, we generate:
+    if (!isPPC64) {
+      // Transforms the ISD::TOC_ENTRY node to a PPCISD::LWZtoc.
----------------
It might help to have a comment before the block to indicate that it handles the rest of the cases for the small code model.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:5100
+      if (isELFABI) {
+          assert (TM.isPositionIndependent() &&
+                 "32-bit ELF can only have TOC entries in position independent"
----------------
Indentation is four spaces in from the surrounding context here (should be two).


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:5106
+          return;
+        } 
+    
----------------
Closing brace does not line up with the corresponding `if`.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:5114
+
+    assert((isPPC64 || (isAIXABI && !isPPC64)) && "Only 64-bit ELF or 32-bit AIX"
+          " support medium/large code model."); 
----------------
Add an assertion before this:
```
assert(CModel != CodeModel::Small);
```



================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:5114
+
+    assert((isPPC64 || (isAIXABI && !isPPC64)) && "Only 64-bit ELF or 32-bit AIX"
+          " support medium/large code model."); 
----------------
hubert.reinterpretcast wrote:
> Add an assertion before this:
> ```
> assert(CModel != CodeModel::Small);
> ```
> 
The English does not match the assertion. The assertion says that either we are dealing with 64-bit (ELF or AIX) or 32-bit AIX.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:5118
+    // Transforms the ISD::TOC_ENTRY node for 32-bit AIX large code model mode
+    // or 64-bit ELF medium or large code model mode. We generate two
+    // instructions as described below.The first source operand is a symbol
----------------
[ ... ] or 64-bit medium (ELF-only) or large (ELF and AIX) code model code. [ ... ]


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:5119
+    // or 64-bit ELF medium or large code model mode. We generate two
+    // instructions as described below.The first source operand is a symbol
+    // reference. If it must be toc-referenced according to PPCSubTarget, we
----------------
Missing space after the period.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:5124
+    //   LWZtocL(@sym, ADDIStocHA(%r2, @sym))
+    // [64-bit ELF]
     //   LDtocL(@sym, ADDIStocHA8(%x2, @sym))
----------------
Please clarify the treatment of 64-bit AIX.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63547