[PATCH] D63547: [AIX]Global Address Lowering

[Sean Fertile via Phabricator via llvm-commits]

sfertile marked an inline comment as done.
sfertile added a comment.

A couple minor comments, I think we are almost there.



================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:5079
+
+    // PowerPC doesn't support tiny or kernel codemodels. For 64-bit medium
+    // and large code model, we generate two instructions as described below.
----------------
Would it be better to document that Tiny and Kernel code models  are not supported with an assert as opposed to a comment? Then we could simplify both the comment and the conditional below to `isPPC64 && CodeModel == CodeModel::Small`, as well as remove the assert following the comment ` // Small code model has been handled.` below.



================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:5120
+    // [32-bit AIX]
+    //   LWZtocL(@sym, ADDIStocHA32(%r2, @sym))
+    // [64-bit ELF]
----------------
minor nit: `LWZtocL(@sym, ADDIStocHA(%r2, @sym))`


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:5122
+    // [64-bit ELF]
     //   LDtocL(@sym, ADDIStocHA(%x2, @sym))
     // Otherwise we generate:
----------------
`LDtocL(@sym, ADDIStocHA8(%x2, @sym))`


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:5124
     // Otherwise we generate:
     //   ADDItocL(ADDIStocHA(%x2, @sym), @sym)
     SDValue GA = N->getOperand(0);
----------------
ditto


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:14314
   CodeModel::Model CModel = getTargetMachine().getCodeModel();
   // If it is small or large code model, module locals are accessed
   // indirectly by loading their address from .toc/.got. The difference
----------------
minor nit: The second half of the comment doesn't really fit in with the rest of the function. Its an implementation detail (which is also explained at the point we perform said lowering). This function is about when we have to add indirection when accessing an operand. I would suggests shortening the comment to:

```
// If it is small or large code model, module locals are accessed
// indirectly by loading their address from .toc/.got.
```




================
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",
----------------
sfertile wrote:
> 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.
You should be able to rebase onto master to pick up the `ADDIStocHA` --> `ADDIStocHA8` patch.


================
Comment at: llvm/lib/Target/PowerPC/PPCTargetMachine.cpp:251
   }
-  if (!TT.isOSDarwin() && !JIT &&
+  if (!TT.isOSAIX() && !TT.isOSDarwin() && !JIT &&
       (TT.getArch() == Triple::ppc64 || TT.getArch() == Triple::ppc64le))
----------------
Xiangling_L wrote:
> sfertile wrote:
> > I suggest we take this chance to split this up. It gets longer, but I think much more readable. Something along the lines of:
> > 
> > 
> > ```
> > if (JIT)
> >   return CodeModel::Small;
> > 
> > if (TT.isOSAIX())
> >   return CodeModel::Small;
> > 
> > assert(TT.isOSBinFormatELF() && "All remaining PPC OSs are ELF based.");
> > 
> > if (TT.isArch32())
> >   return CodeMode::Small;
> > 
> > assert(TT.isArch64() && "Unsupported  PPC architecture.");
> > return CodeModel::Medium;
> > ```
> > 
> > I made the assumption we don't have to support Darwin here anymore. There may be 1 or 2 tests left that use a Darwin target with llvm-mc. If you encounter problems I'll look at changing the test to no longer need darwin if possible. I think we have reached a point where we should try to actively clean up the darwin code.
> Thanks, I updated it. And there is no Darwin LIT testcase failure currently. 
> And there is no Darwin LIT testcase failure currently.

Nice.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63547