[PATCH] D63547: [AIX]Global Address Lowering

Mon Jul 8 11:00:41 PDT 2019

sfertile 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);
----------------
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?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63547