PowerPC: Fix register spilling for SPE registers

[jhibbits]

Summary

Missed in the original commit, use the correct callee-saved register
list for spilling, instead of the standard SVR432 list. This avoids
needlessly spilling the SPE non-volatile registers when they're not used.

[nemanjai]

I think this is very close, but I really think the reloading code needs to be fixed.

lib/Target/PowerPC/PPCInstrInfo.cpp

1161 ↗

(On Diff #181713)

This does not look right. We are in RC == nullptr section here. Why are we checking hasSubClassEq(nullptr)?

lib/Target/PowerPC/PPCRegisterInfo.cpp

163 ↗

(On Diff #181713)

These ternary operator expressions were arguably too complex already. I don't think there's any doubt that after this they're becoming nearly unparsable by a human. Can you please refactor these to use nested if's?

[jhibbits]

Address feedback. Update patch to provide full context.

[nemanjai]

Can you add a test case? At least for the spills that you're adding.

[jhibbits]

In D56703#1449235, @nemanjai wrote:

Can you add a test case? At least for the spills that you're adding.

Is there a good example for a test case testing register spills with no determined register class? That's the only location where I added new spill handling. The real meat of this should already be handled by the existing regression tests.

[jhibbits]

Reduce the scope of the patch.

[jhibbits]

I removed the extra spill changes. Those can be done later if there actually is a problem. This didn't change the tests at all. I'm not quite sure how to write a test to demonstrate that "some but not all" registers are spilled in a given case, given that the register allocator can change at any time, so can't hard-code the registers that would get spilled, and I don't think I can even hard-code the number of registers that could get spilled.

[nemanjai]

LGTM other than the code here looks really messy - but it looks messy regardless of this patch.

I think after this lands, we should refactor this stuff to something like this:

const MCPhysReg *CSRLists[] = {
  CSR_64_AllRegs_VSX_SaveList,
  CSR_64_AllRegs_Altivec_SaveList,
  CSR_64_AllRegs_SaveList,
  CSR_Darwin64_Altivec_SaveList,
  CSR_Darwin64_SaveList,
  CSR_Darwin32_Altivec_SaveList,
  CSR_Darwin32_SaveList,
  ...
};

// Compute the index into the array based on Subtarget features, calling convention, ABI, the need to save R2, and object mode (32/64 bit).
return CSRLists[ComputedIdx];

and similarly for the regmasks.

lib/Target/PowerPC/PPCRegisterInfo.cpp

170 ↗

(On Diff #206986)

No need for else when the previous if just does a return. The same comment applies to all the else statements here.

// Cold calling convention CSR's.
if (MF->getFunction().getCallingConv() == CallingConv::Cold) {
  // 64-bit targets.
  if (TM.isPPC64()) {
    if (Subtarget.hasAltivec())
      return SaveR2 ? ...
    return SaveR2 ? ...
  }
  // 32-bit targets.
  if (Subtarget.hasAltivec())
    return CSR_SVR32_ColdCC_Altivec_SaveList;
  if (Subtarget.hasSPE())
    return CSR_SVR32_ColdCC_SPE_SaveList;
  return CSR_SVR32_ColdCC_SaveList
}

// Standard calling convention CSR's.
...

[nemanjai]

Oops. Forgot to accept.

llvm/trunk/lib/Target/PowerPC/PPCCallingConv.td

 // coldcc calling convection marks most registers as non-volatile.
 // Do not include r1 since the stack pointer is never considered a CSR.
 // Do not include r2, since it is the TOC register and is added depending
 // on whether or not the function uses the TOC and is a non-leaf.
 // Do not include r0,r11,r13 as they are optional in functional linkage
 // and value may be altered by inter-library calls.
 // Do not include r12 as it is used as a scratch register.
 // Do not include return registers r3, f1, v2.
-def CSR_SVR32_ColdCC : CalleeSavedRegs<(add (sequence "R%u", 4, 10),
+def CSR_SVR32_ColdCC_Common : CalleeSavedRegs<(add (sequence "R%u", 4, 10),
                                                 (sequence "R%u", 14, 31),
-                                          F0, (sequence "F%u", 2, 31),
                                                 (sequence "CR%u", 0, 7))>;
 
+def CSR_SVR32_ColdCC : CalleeSavedRegs<(add CSR_SVR32_ColdCC_Common,
+                                          F0, (sequence "F%u", 2, 31))>;
+
+
 def CSR_SVR32_ColdCC_Altivec : CalleeSavedRegs<(add CSR_SVR32_ColdCC,
                                             (sequence "V%u", 0, 1),
                                             (sequence "V%u", 3, 31))>;
 
+def CSR_SVR32_ColdCC_SPE : CalleeSavedRegs<(add CSR_SVR32_ColdCC_Common,
+                                            (sequence "S%u", 4, 10),
+                                            (sequence "S%u", 14, 31))>;
+
 def CSR_SVR64_ColdCC : CalleeSavedRegs<(add  (sequence "X%u", 4, 10),
                                              (sequence "X%u", 14, 31),
                                              F0, (sequence "F%u", 2, 31),
                                              (sequence "CR%u", 0, 7))>;
 
 def CSR_SVR64_ColdCC_R2: CalleeSavedRegs<(add CSR_SVR64_ColdCC, X2)>;
 
 def CSR_SVR64_ColdCC_Altivec : CalleeSavedRegs<(add CSR_SVR64_ColdCC,

llvm/trunk/lib/Target/PowerPC/PPCInstrInfo.cpp

void PPCInstrInfo::copyPhysReg(MachineBasicBlock &MBB,
   else if (PPC::QFRCRegClass.contains(DestReg, SrcReg))
     Opc = PPC::QVFMR;
   else if (PPC::QSRCRegClass.contains(DestReg, SrcReg))
     Opc = PPC::QVFMRs;
   else if (PPC::QBRCRegClass.contains(DestReg, SrcReg))
     Opc = PPC::QVFMRb;
   else if (PPC::CRBITRCRegClass.contains(DestReg, SrcReg))
     Opc = PPC::CROR;
+  else if (PPC::SPE4RCRegClass.contains(DestReg, SrcReg))
+    Opc = PPC::OR;
   else if (PPC::SPERCRegClass.contains(DestReg, SrcReg))
     Opc = PPC::EVOR;
   else
     llvm_unreachable("Impossible reg-to-reg copy");
 
   const MCInstrDesc &MCID = get(Opc);
   if (MCID.getNumOperands() == 3)
     BuildMI(MBB, I, DL, MCID, DestReg)

llvm/trunk/lib/Target/PowerPC/PPCRegisterInfo.cpp

return TM.isPPC64()
                ? (Subtarget.hasAltivec() ? CSR_Darwin64_Altivec_SaveList
                                          : CSR_Darwin64_SaveList)
                : (Subtarget.hasAltivec() ? CSR_Darwin32_Altivec_SaveList
                                          : CSR_Darwin32_SaveList);
 
   if (TM.isPPC64() && MF->getInfo<PPCFunctionInfo>()->isSplitCSR())
     return CSR_SRV464_TLS_PE_SaveList;
 
-  if (Subtarget.hasSPE())
-    return CSR_SVR432_SPE_SaveList;
-
   // On PPC64, we might need to save r2 (but only if it is not reserved).
   bool SaveR2 = MF->getRegInfo().isAllocatable(PPC::X2);
 
+  // Cold calling convention CSRs.
   if (MF->getFunction().getCallingConv() == CallingConv::Cold) {
-    return TM.isPPC64()
-               ? (Subtarget.hasAltivec()
-                      ? (SaveR2 ? CSR_SVR64_ColdCC_R2_Altivec_SaveList
-                                : CSR_SVR64_ColdCC_Altivec_SaveList)
-                      : (SaveR2 ? CSR_SVR64_ColdCC_R2_SaveList
-                                : CSR_SVR64_ColdCC_SaveList))
-               : (Subtarget.hasAltivec() ? CSR_SVR32_ColdCC_Altivec_SaveList
-                                         : CSR_SVR32_ColdCC_SaveList);
+    if (TM.isPPC64()) {
+      if (Subtarget.hasAltivec())
+        return SaveR2 ? CSR_SVR64_ColdCC_R2_Altivec_SaveList
+                      : CSR_SVR64_ColdCC_Altivec_SaveList;
+      return SaveR2 ? CSR_SVR64_ColdCC_R2_SaveList
+                    : CSR_SVR64_ColdCC_SaveList;
     }
-
-  return TM.isPPC64()
-             ? (Subtarget.hasAltivec()
-                    ? (SaveR2 ? CSR_SVR464_R2_Altivec_SaveList
-                              : CSR_SVR464_Altivec_SaveList)
-                    : (SaveR2 ? CSR_SVR464_R2_SaveList : CSR_SVR464_SaveList))
-             : (Subtarget.hasAltivec() ? CSR_SVR432_Altivec_SaveList
-                                       : CSR_SVR432_SaveList);
+    // 32-bit targets.
+    if (Subtarget.hasAltivec())
+      return CSR_SVR32_ColdCC_Altivec_SaveList;
+    else if (Subtarget.hasSPE())
+      return CSR_SVR32_ColdCC_SPE_SaveList;
+    return CSR_SVR32_ColdCC_SaveList;
+  }
+  // Standard calling convention CSRs.
+  if (TM.isPPC64()) {
+    if (Subtarget.hasAltivec())
+      return SaveR2 ? CSR_SVR464_R2_Altivec_SaveList
+                    : CSR_SVR464_Altivec_SaveList;
+    return SaveR2 ? CSR_SVR464_R2_SaveList
+                  : CSR_SVR464_SaveList;
+  }
+  // 32-bit targets.
+  if (Subtarget.hasAltivec())
+    return CSR_SVR432_Altivec_SaveList;
+  else if (Subtarget.hasSPE())
+    return CSR_SVR432_SPE_SaveList;
+  return CSR_SVR432_SaveList;
 }
 
 const MCPhysReg *
 PPCRegisterInfo::getCalleeSavedRegsViaCopy(const MachineFunction *MF) const {
   assert(MF && "Invalid MachineFunction pointer.");
   const PPCSubtarget &Subtarget = MF->getSubtarget<PPCSubtarget>();
   if (Subtarget.isDarwinABI())
     return nullptr;
if (Subtarget.isAIXABI()) {
     assert(!Subtarget.hasAltivec() && "Altivec is not implemented on AIX yet.");
     return TM.isPPC64() ? CSR_AIX64_RegMask : CSR_AIX32_RegMask;
   }
 
   if (CC == CallingConv::Cold) {
     return TM.isPPC64() ? (Subtarget.hasAltivec() ? CSR_SVR64_ColdCC_Altivec_RegMask
                                                   : CSR_SVR64_ColdCC_RegMask)
                         : (Subtarget.hasAltivec() ? CSR_SVR32_ColdCC_Altivec_RegMask
-                                                  : CSR_SVR32_ColdCC_RegMask);
+                                                  : (Subtarget.hasSPE()
+                                                  ? CSR_SVR32_ColdCC_SPE_RegMask
+                                                  : CSR_SVR32_ColdCC_RegMask));
   }
 
   return TM.isPPC64() ? (Subtarget.hasAltivec() ? CSR_SVR464_Altivec_RegMask
                                                 : CSR_SVR464_RegMask)
                       : (Subtarget.hasAltivec() ? CSR_SVR432_Altivec_RegMask
-                                                : CSR_SVR432_RegMask);
+                                                : (Subtarget.hasSPE()
+                                                  ? CSR_SVR432_SPE_RegMask
+                                                  : CSR_SVR432_RegMask));
 }
 
 const uint32_t*
 PPCRegisterInfo::getNoPreservedMask() const {
   return CSR_NoRegs_RegMask;
 }
 
 void PPCRegisterInfo::adjustStackMapLiveOutMask(uint32_t *Mask) const {