[PATCH] D63547: [AIX]Global Address Lowering

[Stefan Pintilie via Phabricator via llvm-commits]

stefanp added a comment.

You have convinced me that this will only affect AIX. I only have minor comments remaining.



================
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:
> 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.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:5115
+      SDNode *MN = CurDAG->getMachineNode((isPPC64) ? PPC::LDtocL :
+                                          PPC::LWZtocL, dl, VT, GA, 
                                           SDValue(Tmp, 0));
----------------
Xiangling_L wrote:
> 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.
Ok, fair enough. I think this is fine.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63547