[PATCH] D63547: [AIX]Global Address Lowering

[Stefan Pintilie via Phabricator via llvm-commits]

stefanp added subscribers: jhibbits, joerg, stefanp.
stefanp added a comment.

I feel like this is supposed to be an AIX only patch. 
However, there are some cases where you are making changes to PowerPC platforms that are not AIX. I don't know if those changes are safe. 
I'm going to add two people to the review who may be interested in this and who work on 32 Bit PowerPC.

FYI:   @jhibbits @joerg



================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:5071
+    bool isSVR4ABI = PPCSubTarget->isSVR4ABI();
+    bool isAIXABI = PPCSubTarget->isAIXABI();
+
----------------
minor nit:
These three can be `const`.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:5108
+    SDNode *Tmp = CurDAG->getMachineNode((isPPC64) ? PPC::ADDIStocHA : 
+                                         PPC::ADDIStocHA32, dl, VT, TOCbase, 
+                                         GA);
----------------
Here you may be making changes to 32 bit PowerPC platforms that are not AIX.


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


================
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();
----------------
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. 


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




================
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))
----------------

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63547