[PATCH] D34815: [Power9] Spill gprs to vector registers rather than stack

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added a comment.

Other than a few minor inline comments, this LGTM.

Perhaps some of the other reviewers want to chime in on this. Otherwise please address those nits and commit.



================
Comment at: lib/Target/PowerPC/PPCInstrInfo.cpp:934
+    return;
+  } else if (PPC::G8RCRegClass.contains(DestReg) &&
+             PPC::VSFRCRegClass.contains(SrcReg)) {
----------------
I think for most (all?) other conditions, we have the source first. Please stick to that convention here as well.


================
Comment at: lib/Target/PowerPC/PPCInstrInfo.cpp:2036
+  case PPC::SPILLTOVSR_ST: {
+    unsigned TargetReg = MI.getOperand(0).getReg();
+    if (PPC::VSFRCRegClass.contains(TargetReg)) {
----------------
Just a nit. The register you're spilling is the **source** and the stack slot you're spilling it to is the **target**. So calling it `TargetReg` is a bit misleading when it's a store. :)


================
Comment at: lib/Target/PowerPC/PPCRegisterInfo.cpp:54
+STATISTIC(InflateGPRC, "Number of gprc inputs for getLargestLegalClass");
+STATISTIC(InflateGP8RC, "Number of gp8rc inputs for getLargestLegalClass");
+
----------------
Nit: it's not actually called `gp8rc` but `g8rc` if I remember correctly.


================
Comment at: lib/Target/PowerPC/PPCRegisterInfo.cpp:341
 
+    // For pwr9 we enable gpr to vector spills
+    // FIXME: Currently limited to spilling GP8RC. A follow on patch will add
----------------
`// For Power9 we allow the user to enable GPR to vector spills.`

Since we don't currently enable it by default even on Power9.


================
Comment at: lib/Target/PowerPC/PPCRegisterInfo.cpp:344
+    // support to spill GPRC.
+    if (Subtarget.hasP9Vector() && EnableGPRToVecSpills &&
+        RC == &PPC::G8RCRegClass) {
----------------
Please add a check for ELFv2 ABI. We are allowing spills only to the volatile VSR's, so we want to enable this only on the ABI where the VSR's we've selected are actually volatile.


================
Comment at: lib/Target/PowerPC/PPCRegisterInfo.td:308
 
+
+def SPILLTOVSRRC : RegisterClass<"PPC", [i64, f64], 64, (add G8RC, (sub VSFRC,
----------------
`// Allow spilling GPR's into caller-saved VSR's.`


================
Comment at: test/CodeGen/PowerPC/gpr-vsr-spill2.ll:1
+; RUN: llc -verify-machineinstrs -mcpu=pwr9 -ppc-enable-gpr-to-vsr-spills  < %s | FileCheck %s
+; Testcase narrowed down from SPEC to highlight corner case of spilling the new reg class.
----------------
As implemented, this test case doesn't really test anything meaningful. It really just tests that there's a reg-to-reg copy (implemented as a move-register) followed by a spill of the target register. The two could be separated by arbitrary amount of code (including redefinition of the register).

Unless you can add more meaningful testing to this complicated test case, I would simply get rid of it.


https://reviews.llvm.org/D34815