[PATCH] D63547: [AIX]Global Address Lowering

Tue Jul 9 07:39:29 PDT 2019

Xiangling_L marked an inline comment as done.
Xiangling_L added inline comments.

================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:5093
+    if (isAIXABI && CModel == CodeModel::Medium) 
+      report_fatal_error("Medium code model is not supported on AIX.");
+
----------------
hubert.reinterpretcast wrote:
> Just as a note: GCC on AIX generates large code model relocations for `-mcmodel=medium`. I guess that should get handled at the driver level.
Thanks for mentioning that, I will open an internal issue as a reminder.

================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:5108
+    SDNode *Tmp = CurDAG->getMachineNode((isPPC64) ? PPC::ADDIStocHA : 
+                                         PPC::ADDIStocHA32, dl, VT, TOCbase, 
+                                         GA);
----------------
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?
```
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.

================
Comment at: llvm/lib/Target/PowerPC/PPCTOCRegDeps.cpp:117

-        MI.addOperand(MachineOperand::CreateReg(PPC::X2,
+        MI.addOperand(MachineOperand::CreateReg(TOCReg,
                                                 false  /*IsDef*/,
----------------
stefanp wrote:
> Xiangling_L wrote:
> > stefanp wrote:
> > > I assume this patch is only meant to affect AIX.
> > > If that is the case I'm afraid that this line might actually change the behaviour of non-AIX PowerPC builds for 32Bit. Should we add an AIX guard to this too? 
> > > 
> > > 
> > Thanks for pointing this out. As my understanding, this file is adding a pass to resolve a r2 dependency issue related to ELF ABI under large code model where we use "toc at ha" and "toc at l" to access the toc entry. 
> > 
> > And on PPC under SVR4 ABI, or ELF SVR4 ABI more specifically, little-endien ELF ABI doesn't have 32-bit, and even if big-endien ELF ABI has 32-bit, it doesn't have large code model, which means speaking of ELF ABI on PPC, there is no 32-bit large code model. 
> > 
> > So I think my patch will only affect AIX 32bit large code model here. 
> > 
> > Feel free to raise your further concerns. And maybe I can add an assertion or change it to (is64Bit? && isAIXABI()) for more clarity purpose?
> I was not aware that this was only for large code model. Ok, I'm convinced that this patch does not affect anything but AIX.
> You could add an assert here because it is not obvious why this is only for AIX 32bit.
sure, I will update it.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63547