[PATCH] D63547: [AIX]Global Address Lowering

[Xiangling Liao via Phabricator via llvm-commits]

Xiangling_L marked 11 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);
----------------
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?


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:5115
+      SDNode *MN = CurDAG->getMachineNode((isPPC64) ? PPC::LDtocL :
+                                          PPC::LWZtocL, dl, VT, GA, 
                                           SDValue(Tmp, 0));
----------------
stefanp wrote:
> Here you may be making changes to 32 bit PowerPC platforms that are not AIX. Previously all PowerPC platforms got this: `PPC::LDtocL` now the 32 bit ones get `PPC::LWZtocL`  even if that 32 bit platform is not AIX. 
The same reason as above. It will affect only AIX platform. Besides, I think PPC little endien SVR4 ABI doesn't have 32bit mode, and PPC big endien 32bit have small code model only.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:2642
+static SDValue getTOCEntry(SelectionDAG &DAG, const SDLoc &dl,
+                           const PPCSubtarget &Subtarget, SDValue GA) {
+  bool Is64Bit = Subtarget.isPPC64();
----------------
stefanp wrote:
> You don't need to pass the subtarget into this function.
> ```
> DAG.getSubtarget() 
> ```
> You already have the `DAG` so you can get the subtarget that way. 
Thanks. I will update this.


================
Comment at: llvm/lib/Target/PowerPC/PPCTOCRegDeps.cpp:117
 
-        MI.addOperand(MachineOperand::CreateReg(PPC::X2,
+        MI.addOperand(MachineOperand::CreateReg(TOCReg,
                                                 false  /*IsDef*/,
----------------
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?


================
Comment at: llvm/lib/Target/PowerPC/PPCTOCRegDeps.cpp:134
+        bool Is64Bit = MF.getSubtarget<PPCSubtarget>().isPPC64();
+        unsigned TOCReg = Is64Bit ? PPC::X2 : PPC::R2;
+        if (processBlock(B, TOCReg))
----------------
stefanp wrote:
> 
> I think it would be better to compute TOCReg in processBlock. That might simplify the change a little.
> ```
> const bool Is64Bit = MBB.getParent()->getSubtarget<PPCSubtarget>().isPPC64();
> const unsigned TOCReg = Is64Bit ? PPC::X2 : PPC::R2;
> ```
> If you do it this way you don't have to pass in more parameters.
Thanks, I will update this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63547