[PATCH] D63547: [AIX]Global Address Lowering

[Xiangling Liao via Phabricator via llvm-commits]

Xiangling_L marked 3 inline comments as done.
Xiangling_L added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:5108
+    SDNode *Tmp = CurDAG->getMachineNode((isPPC64) ? PPC::ADDIStocHA : 
+                                         PPC::ADDIStocHA32, dl, VT, TOCbase, 
+                                         GA);
----------------
Xiangling_L wrote:
> sfertile wrote:
> > sfertile wrote:
> > > stefanp wrote:
> > > > Xiangling_L wrote:
> > > > > stefanp wrote:
> > > > > > Here you may be making changes to 32 bit PowerPC platforms that are not AIX.
> > > > > I think the case "PPCISD::TOC_ENTRY" will only deal with SVR4 ABI [32/64bit] + AIX ABI[32/64bit] + Darwin[64bit] like the assertion implies.
> > > > > 
> > > > >  And the only parts that will touch the code from line 5106-5109 were 64bit SVR4, 64bit Darwin previously and are 64bit SVR4, 64bit Darwin plus 32bit AIX now. So It only makes changes to 32bit PowerPC AIX platform. 
> > > > > 
> > > > > Maybe I can try to put an assertion here for more clarity?
> > > > Good point. I missed that.
> > > > I don't think you need an assertion. What you have here is fine.
> > > I think there is a lot of implicit understanding baked into this code. There were similar comments in a downstream review before posting to phabricator, and now your comment. My understanding was the same as Xianglings based of the assert, i.e. subtarget could be any of the 64-bit ones (Darwin/ELFV1/ELFV2) or 32-bit ELF.
> > > 
> > > I dug into this a bit deeper to try to verify my understanding. From what I now understand by looking at where we create TOC_ENTRY in ISELLowering (LowerGlobalAddress/LowerConstantPool/LowerJumpTable/LowerBlockAddress)  we only create a TOC entry for
> > > 
> > > 64-bit ELF 
> > > 32-bit position-independent ELF  (-fPIC or -fPIE)
> > > 
> > > clang accepts -mcmodel=medium/large for 32-bit ELF target but we always produce a small code model TOC access. (the system gcc I'm using  produces `error: -mcmodel not supported in this configuration` when trying to set the code model with -m32).
> > > 
> > > I think based on the number of people making the same comments, and both me and Xiangling coming up with the same misunderstanding of what targets reach here we should restructure the code to better document these realities.
> > > 
> > > 
> > > The one question I am left with is why we use a single TOC Pseudo for 32-bit small code model (`LWZtoc`) but have `LDtoc`/'LDtocJTI`/`LDtocCPT`/`LDtocBA` for 64-bit small code model?
> > What do you think if we structured the code along these lines:
> > 
> > ```
> > case PPCISD::TOC_ENTRY: {
> >   assert(!isDarwin() && "TOC is an ELF/XCOFF construct");
> > 
> >   // 64-bit small codemodel is handled by `SelectCommonCode`.
> >   if (is64BitELF() && CodeModel == small)
> >     break;
> > 
> >     // Transforms the ISD::TOC_ENTRY node to a PPCISD::LWZtoc
> >     auto replaceWithLWZtoc = [this, dl](SDNode *TocEntry) {
> >       SDValue SymOperand = TocEntry->getOperand(0);
> >       SDValue TocBase = TocEntry->getOperand(1);
> >       SDNode *MN = CurDAG->getMachineNode(PPC::LWZtoc, dl, MVT::i32, SymOperand,
> >                                           TocBase);
> >       transferMemOperands(TocEntry, MN);
> >       ReplaceNode(TocEntry, MN);
> >     };
> > 
> > 
> >   if (is32BitELF()) {
> >     assert(isPositionIndependent() &&
> >             "32-bit ELF can only have TOC entries in position independant code.");
> >     // 32-bit ELF always uses a small codemodel toc access.
> >     replaceWithLWZtoc(N);
> >     return;
> >   }
> > 
> >   if (is32BitAIX() && CodeModel == Small) {
> >     replaceWithLWZtoc(N);
> >     return;
> >   }
> > 
> >   // Small code model has been handled, and PowerPC doesn't support tiny or kernel codemodels.
> >   assert(CodeModel == Medium || CodeModel == Large && "Unexpected code model");
> >   if (isAIX() && CodeModel == Medium)
> >     report_fatal_error("Medium code model is not supported on AIX.");
> > 
> >  // On 64-bit ELF we may be able to access the operand relative to the TOC base (in medium codemodel),
> >  // in which case we produce ADDItocL(ADDIStocHA(%x2, @sym), @sym).
> >  // Otherwise, we access the operand indirectly.
> >  // In 64-bit:
> >  //   LDtocL(@sym, ADDIStocHA(%x2, @sym))
> >  // In 32-bit:
> >  //   LWZtocL(@sym, ADDIStocHA32(%r2, @sym))
> >  ... Everything below here stays the same ...
> > }
> > 
> > ```
> ```
> The one question I am left with is why we use a single TOC Pseudo for 32-bit small code model (LWZtoc) but have LDtoc/'LDtocJTI`/LDtocCPT/LDtocBA for 64-bit small code model?
> ```
> 
> I think I kinda know why where are four opcodes for LDtoc but only one for LWZtoc.
> 
> It's that under 32bit mode, small/medium/large code model essentially all produce small code model results, so in TOC_ENTRY, we manually address this reality by setting opcode to LWZtoc. But for 64bit model, we only need to manually deal with medium and large code model, and we can leave small code model to table gen. So for different lowering targets[eg. global address, contant pool,...] table gen needs one opcode for each to do the lowering.
Thank you for your suggestion. I was trying to restructure the code, and I will see if I can adapt your code here to make it better.


================
Comment at: llvm/lib/Target/PowerPC/PPCTOCRegDeps.cpp:118
+      const bool isAIXABI = Subtarget.isAIXABI();
+      assert(((isPPC64 && isSVR4ABI) || isAIXABI) &&
+           "Only supported for 64-bit SVR4 ABI and AIX ABI under large code "
----------------
sfertile wrote:
> Does it help readability if we add some extra helpers to the subtarget combining ABI and pointer width checks?
> eg `is64BitELF()`, `is32BitAIX()` etc? 
or we can do?:

```
assert(((isPPC64 && isTargetELF()) || isAIXABI) &&...
```
and 
```
const bool is32BitAIX = !isPPC64 && isAIXABI;
const unsigned TOCReg = (is32BitAIX) ? PPC::X2 : PPC::R2;
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63547