[WIP] Cleanup SplitCSR implementation

[reames]

Summary

Very much WIP, posting for discussion, will cleanup based on input.

Key question: Is minimal phys reg the right helper routine to be using here? The ARM backend change makes me wonder. It does match what the x86 backend uses for non-GPR spills along the "normal" spill insertion path.

SplitCSR is an off by default mode which let's the register allocator handle spilling CSRs as normal vregs. This really should be the default, but for historical reasons isn't.

This change tries to remove the target specific differences from the copied implementations. Once this is in, a simple NFC change will sink a shared implementation into the base class and delete all of the overrides.

[MatzeB]

I think getMinimalPhysRegClass() may be more restrictive than necessary (I imagine you can end up the class for A B C D as the smallest class on X86 while the full GPR class would be more suitable on X86). I don't think we have a better generic notion today, so all that is left is to enumerate register classes that allow suitable spilling and cheaply copying between the registers inside the class.

There is also some bad code duplication going on here today. I think we solve the same (or really similar) problem in LowerFormalArguments() and LowerReturn().

[MatzeB]

In D27293#610230, @MatzeB wrote:

I think getMinimalPhysRegClass() may be more restrictive than necessary (I imagine you can end up the class for A B C D as the smallest class on X86 while the full GPR class would be more suitable on X86). I don't think we have a better generic notion today, so all that is left is to enumerate register classes that allow suitable spilling and cheaply copying between the registers inside the class.

That said I think I would be fine to use getMinimalPhysRegClass() for the cases not covered by the explicit enumeration.

[qcolombet]

[speaking of getMinimanRCForPhysReg] I don't think we have a better generic notion today,

I concur, we don't have a better generic notion.

so all that is left is to enumerate register classes that allow suitable spilling and cheaply copying between the registers inside the class.

I would expect this to be a non-problem. The splitting mechanism should relax the constraints if need be. Therefore, yes, at first it will be overconstrained, but when split, it will not. The bottom line is that I am not sure the added enumeration is worth bothering, but this is a trade-off.
I don't mind either way.

lib/Target/ARM/ARMISelLowering.cpp

13381

Put the dump changes into a different patch.

lib/Target/X86/X86ISelLowering.cpp

33766

I would rather you get to the bottom of this than having this comment.
Check with Manman.

[reames]

Don't have time to get back to this.

lib/Target/AArch64/AArch64ISelLowering.cpp

void AArch64TargetLowering::insertCopiesSplitCSR(
   const MCPhysReg *IStart = TRI->getCalleeSavedRegsViaCopy(Entry->getParent());
   if (!IStart)
     return;
 
   const TargetInstrInfo *TII = Subtarget->getInstrInfo();
   MachineRegisterInfo *MRI = &Entry->getParent()->getRegInfo();
   MachineBasicBlock::iterator MBBI = Entry->begin();
   for (const MCPhysReg *I = IStart; *I; ++I) {
-    const TargetRegisterClass *RC = nullptr;
-    if (AArch64::GPR64RegClass.contains(*I))
-      RC = &AArch64::GPR64RegClass;
-    else if (AArch64::FPR64RegClass.contains(*I))
-      RC = &AArch64::FPR64RegClass;
-    else
-      llvm_unreachable("Unexpected register class in CSRsViaCopy!");
+    const TargetRegisterClass *RC = TRI->getMinimalPhysRegClass(*I);
 
     unsigned NewVR = MRI->createVirtualRegister(RC);
     // Create copy from CSR to a virtual register.
     // FIXME: this currently does not emit CFI pseudo-instructions, it works
     // fine for CXX_FAST_TLS since the C++-style TLS access functions should be
     // nounwind. If we want to generalize this later, we may need to emit
     // CFI pseudo-instructions.
     assert(Entry->getParent()->getFunction()->hasFnAttribute(

lib/Target/ARM/ARMBaseRegisterInfo.cpp

#endif // NDEBUG
   assert(!MI.isDebugValue() && "DBG_VALUEs should be handled in target-independent code");
 
   // Modify MI as necessary to handle as much of 'Offset' as possible
   bool Done = false;
   if (!AFI->isThumbFunction())
     Done = rewriteARMFrameIndex(MI, FIOperandNum, FrameReg, Offset, TII);
   else {
     assert(AFI->isThumb2Function());
+    MI.dump();
+    dbgs() << FIOperandNum << "\n";
+    dbgs() << FrameReg << "\n";
+    dbgs() << Offset << "\n";
     Done = rewriteT2FrameIndex(MI, FIOperandNum, FrameReg, Offset, TII);
   }
   if (Done)
     return;
 
   // If we get here, the immediate doesn't fit into the instruction.  We folded
   // as much as possible above, handle the rest, providing a register that is
   // SP+LargeImm.

lib/Target/ARM/ARMISelLowering.cpp

void ARMTargetLowering::insertCopiesSplitCSR(
   const MCPhysReg *IStart = TRI->getCalleeSavedRegsViaCopy(Entry->getParent());
   if (!IStart)
     return;
 
   const TargetInstrInfo *TII = Subtarget->getInstrInfo();
   MachineRegisterInfo *MRI = &Entry->getParent()->getRegInfo();
   MachineBasicBlock::iterator MBBI = Entry->begin();
   for (const MCPhysReg *I = IStart; *I; ++I) {
-    const TargetRegisterClass *RC = nullptr;
-    if (ARM::GPRRegClass.contains(*I))
-      RC = &ARM::GPRRegClass;
-    else if (ARM::DPRRegClass.contains(*I))
-      RC = &ARM::DPRRegClass;
-    else
-      llvm_unreachable("Unexpected register class in CSRsViaCopy!");
+    const TargetRegisterClass *RC = TRI->getMinimalPhysRegClass(*I);
+    assert(RC);
 
     unsigned NewVR = MRI->createVirtualRegister(RC);
     // Create copy from CSR to a virtual register.
     // FIXME: this currently does not emit CFI pseudo-instructions, it works
     // fine for CXX_FAST_TLS since the C++-style TLS access functions should be
     // nounwind. If we want to generalize this later, we may need to emit
     // CFI pseudo-instructions.
     assert(Entry->getParent()->getFunction()->hasFnAttribute(
                Attribute::NoUnwind) &&
            "Function should be nounwind in insertCopiesSplitCSR!");
     Entry->addLiveIn(*I);
     BuildMI(*Entry, MBBI, DebugLoc(), TII->get(TargetOpcode::COPY), NewVR)
-        .addReg(*I);
+      .addReg(*I)->dump();
 
     // Insert the copy-back instructions right before the terminator.
     for (auto *Exit : Exits)
       BuildMI(*Exit, Exit->getFirstTerminator(), DebugLoc(),
               TII->get(TargetOpcode::COPY), *I)
           .addReg(NewVR);
   }
 }

lib/Target/ARM/Thumb2InstrInfo.cpp

if (AddrMode == ARMII::AddrModeT2_i8 || AddrMode == ARMII::AddrModeT2_i12) {
       }
     } else if (AddrMode == ARMII::AddrModeT2_i8s4) {
       Offset += MI.getOperand(FrameRegIdx + 1).getImm() * 4;
       NumBits = 10; // 8 bits scaled by 4
       // MCInst operand expects already scaled value.
       Scale = 1;
       assert((Offset & 3) == 0 && "Can't encode this offset!");
     } else {
-      llvm_unreachable("Unsupported addressing mode!");
+      return false;
     }
 
     if (NewOpc != Opcode)
       MI.setDesc(TII.get(NewOpc));
 
     MachineOperand &ImmOp = MI.getOperand(FrameRegIdx+1);
 
     // Attempt to fold address computation

lib/Target/PowerPC/PPCISelLowering.cpp

void PPCTargetLowering::insertCopiesSplitCSR(
   const MCPhysReg *IStart = TRI->getCalleeSavedRegsViaCopy(Entry->getParent());
   if (!IStart)
     return;
 
   const TargetInstrInfo *TII = Subtarget.getInstrInfo();
   MachineRegisterInfo *MRI = &Entry->getParent()->getRegInfo();
   MachineBasicBlock::iterator MBBI = Entry->begin();
   for (const MCPhysReg *I = IStart; *I; ++I) {
-    const TargetRegisterClass *RC = nullptr;
-    if (PPC::G8RCRegClass.contains(*I))
-      RC = &PPC::G8RCRegClass;
-    else if (PPC::F8RCRegClass.contains(*I))
-      RC = &PPC::F8RCRegClass;
-    else if (PPC::CRRCRegClass.contains(*I))
-      RC = &PPC::CRRCRegClass;
-    else if (PPC::VRRCRegClass.contains(*I))
-      RC = &PPC::VRRCRegClass;
-    else
-      llvm_unreachable("Unexpected register class in CSRsViaCopy!");
+    const TargetRegisterClass *RC = TRI->getMinimalPhysRegClass(*I);
 
     unsigned NewVR = MRI->createVirtualRegister(RC);
     // Create copy from CSR to a virtual register.
     // FIXME: this currently does not emit CFI pseudo-instructions, it works
     // fine for CXX_FAST_TLS since the C++-style TLS access functions should be
     // nounwind. If we want to generalize this later, we may need to emit
     // CFI pseudo-instructions.
     assert(Entry->getParent()->getFunction()->hasFnAttribute(

lib/Target/X86/X86ISelLowering.h

private:
     SDValue LowerCall(CallLoweringInfo &CLI,
                       SmallVectorImpl<SDValue> &InVals) const override;
 
     SDValue LowerReturn(SDValue Chain, CallingConv::ID CallConv, bool isVarArg,
                         const SmallVectorImpl<ISD::OutputArg> &Outs,
                         const SmallVectorImpl<SDValue> &OutVals,
                         const SDLoc &dl, SelectionDAG &DAG) const override;
 
-    bool supportSplitCSR(MachineFunction *MF) const override {
-      return MF->getFunction()->getCallingConv() == CallingConv::CXX_FAST_TLS &&
-          MF->getFunction()->hasFnAttribute(Attribute::NoUnwind);
-    }
+    bool supportSplitCSR(MachineFunction *MF) const override;
     void initializeSplitCSR(MachineBasicBlock *Entry) const override;
     void insertCopiesSplitCSR(
       MachineBasicBlock *Entry,
       const SmallVectorImpl<MachineBasicBlock *> &Exits) const override;
 
     bool isUsedByReturnOnly(SDNode *N, SDValue &Chain) const override;
 
     bool mayBeEmittedAsTailCall(CallInst *CI) const override;

lib/Target/X86/X86ISelLowering.cpp

bool X86TargetLowering::isIntDivCheap(EVT VT, AttributeSet Attr) const {
   // integer division, leaving the division as-is is a loss even in terms of
   // size, because it will have to be scalarized, while the alternative code
   // sequence can be performed in vector form.
   bool OptSize = Attr.hasAttribute(AttributeSet::FunctionIndex,
                                    Attribute::MinSize);
   return OptSize && !VT.isVector();
 }
 
-void X86TargetLowering::initializeSplitCSR(MachineBasicBlock *Entry) const {
+bool X86TargetLowering::supportSplitCSR(MachineFunction *MF) const {
+  // Why is this requirement here?
   if (!Subtarget.is64Bit())
-    return;
+    return false;
+  
+  // TODO: SplitCSR does not support CFI at this time.
+  if (MF->getTarget().getMCAsmInfo()->usesWindowsCFI() ||
+      !MF->getFunction()->hasFnAttribute(Attribute::NoUnwind) ||
+      MF->getMMI().hasDebugInfo())
+    return false;
+
+  // This check is probably no longer needed; it was left to avoid mixing
+  // semantic and non-semantic changes.  TODO: consider removing this
+  return MF->getFunction()->getCallingConv() == CallingConv::CXX_FAST_TLS;
+}
+
+void X86TargetLowering::initializeSplitCSR(MachineBasicBlock *Entry) const {
+  assert(supportSplitCSR(Entry->getParent()));
   
   // Update IsSplitCSR in X86MachineFunctionInfo.
   X86MachineFunctionInfo *AFI =
     Entry->getParent()->getInfo<X86MachineFunctionInfo>();
   AFI->setIsSplitCSR(true);
 }
 
 void X86TargetLowering::insertCopiesSplitCSR(
     MachineBasicBlock *Entry,
     const SmallVectorImpl<MachineBasicBlock *> &Exits) const {
+  MachineFunction *MF = Entry->getParent();
+  assert(supportSplitCSR(MF));
+  
   const X86RegisterInfo *TRI = Subtarget.getRegisterInfo();
-  const MCPhysReg *IStart = TRI->getCalleeSavedRegsViaCopy(Entry->getParent());
+  const MCPhysReg *IStart = TRI->getCalleeSavedRegsViaCopy(MF);
   if (!IStart)
     return;
 
   const TargetInstrInfo *TII = Subtarget.getInstrInfo();
   MachineRegisterInfo *MRI = &Entry->getParent()->getRegInfo();
   MachineBasicBlock::iterator MBBI = Entry->begin();
   for (const MCPhysReg *I = IStart; *I; ++I) {
-    const TargetRegisterClass *RC = nullptr;
-    if (X86::GR64RegClass.contains(*I))
-      RC = &X86::GR64RegClass;
-    else
-      llvm_unreachable("Unexpected register class in CSRsViaCopy!");
-
-    unsigned NewVR = MRI->createVirtualRegister(RC);
+    const TargetRegisterClass *RC = TRI->getMinimalPhysRegClass(*I);
+    const unsigned NewVR = MRI->createVirtualRegister(RC);
     // Create copy from CSR to a virtual register.
     // FIXME: this currently does not emit CFI pseudo-instructions, it works
     // fine for CXX_FAST_TLS since the C++-style TLS access functions should be
     // nounwind. If we want to generalize this later, we may need to emit
     // CFI pseudo-instructions.
-    assert(Entry->getParent()->getFunction()->hasFnAttribute(
-               Attribute::NoUnwind) &&
+    // TODO: factor this out into a more precise check: see the logic in
+    // X86FrameLowering.cpp around windows, dwarf, etc..
+    assert(MF->getFunction()->hasFnAttribute(Attribute::NoUnwind) &&
            "Function should be nounwind in insertCopiesSplitCSR!");
     Entry->addLiveIn(*I);
     BuildMI(*Entry, MBBI, DebugLoc(), TII->get(TargetOpcode::COPY), NewVR)
         .addReg(*I);
 
     // Insert the copy-back instructions right before the terminator.
     for (auto *Exit : Exits)
       BuildMI(*Exit, Exit->getFirstTerminator(), DebugLoc(),
               TII->get(TargetOpcode::COPY), *I)
           .addReg(NewVR);
   }
 }
 
 bool X86TargetLowering::supportSwiftError() const {
   return Subtarget.is64Bit();
 }