[PATCH] D74486: [PowerPC][Future] Add initial support for PC Relative addressing for constant pool loads

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai accepted this revision.
nemanjai added a comment.
This revision is now accepted and ready to land.

LGTM aside from some minor nits. Feel free to address those on the commit.



================
Comment at: llvm/lib/Target/PowerPC/PPC.h:103
+    /// the current instruction address(pc), e.g., var at pcrel. Fixup is VK_PCREL.
+    MO_PCREL_FLAG = 16,
+
----------------
Please add to the comment the reasoning behind this being 16 rather than the next power of 2.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:2769
+                                                    PPCII::MO_PCREL_FLAG);
+      SDValue MatAddr = DAG.getNode(PPCISD::MAT_PCREL_ADDR, DL, Ty, ConstPool);
+      return MatAddr;
----------------
Just `return DAG.getNode(...);`


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.td:326
+// PC Relative Specific Nodes
+def MATpcreladdr : SDNode<"PPCISD::MAT_PCREL_ADDR", SDTIntUnaryOp, []>;
+
----------------
All the other PPC-specific nodes start with `PPC`. Please maintain this convention.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrPrefix.td:347
+// instruction selection to consistently pick these first without the current
+// added complexity. For the future we should refactor the addessing selection
+// on PowerPC so that this AddedComplexity=500 will not be required.
----------------
This sounds like wishful thinking and not like a commitment to provide this refactoring in the near future. Please change it to:
```
// Once pc-relative implementation is complete, a set of follow-up patches will
// address this refactoring and the AddedComplexity will be removed.
```


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrPrefix.td:351
+  // Load f32
+  def : Pat<(f32 (load (MATpcreladdr pcreladdr:$cp))), (PLFSpc $cp, 0)>;
+
----------------
Nit: throughout this block, the name of the pc-relative address seems to suggest that this is only for constant pool entries (i.e. `cp`). However, this is presumably useful for any pc-relative memory operations. Perhaps `$addr` or `$src`.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrPrefix.td:355
+  def : Pat<(f64 (extloadf32 (MATpcreladdr pcreladdr:$cp))),
+              (COPY_TO_REGCLASS (PLFSpc $cp, 0), VSFRC)>;
+  def : Pat<(f64 (load (MATpcreladdr pcreladdr:$cp))), (PLFDpc $cp, 0)>;
----------------
Nit, the alignment is wrong here. When splitting on two lines, the input and output patterns should be aligned in the same column.


================
Comment at: llvm/test/CodeGen/PowerPC/csr-split.ll:3
 ; RUN: llc -verify-machineinstrs  -ppc-vsr-nums-as-vr -ppc-asm-full-reg-names \
+; RUN:     -mtriple=powerpc64le-unknown-linux-gnu -mcpu=future < %s | FileCheck %s --check-prefix=CHECK-FUTURE
+; RUN: llc -verify-machineinstrs  -ppc-vsr-nums-as-vr -ppc-asm-full-reg-names \
----------------
What is the addition to this test meant to accomplish? I do not see any constant pool loads in the test - or anything related to this patch TBH.


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

https://reviews.llvm.org/D74486