Consider CSRs in computeRegisterLiveness

[arsenm]

Summary

I'm not sure how to test this. The AMDGPU use
case registers aren't CSRs.

ARM is the only other user, and mostly uses it for
CPSR which also isn't a CSR.

The use in getImplicitSPRUseForDPRUse seems like
it may be possible to use a CSR here, but so far
I don't know enough about ARM to come up with an
example where this matters.

[MatzeB]

Thanks for looking into this!

lib/CodeGen/MachineBasicBlock.cpp

1412–1418

• Thinking about it, the CSRs are only considered live-out of the return block after prolog epilog insertion, so the whole code here needs to be guarded with MFI.isCalleeSavedInfoValid().
• CalleeeSavedInfo() must be a subset of MRI.getCalleeSavedRegs() so we shouldn't need the 2nd loop.

[MatzeB]

lib/CodeGen/MachineBasicBlock.cpp

1412–1418

CalleeeSavedInfo() must be a subset of MRI.getCalleeSavedRegs() so we shouldn't need the 2nd loop.

Ignore this 2nd part of my comment. I just realized we can have callee saved registers that are not live-out of the return block...

[arsenm]

Only check if MFI.isCalleeSavedInfoValid

[thegameg]

lib/CodeGen/MachineBasicBlock.cpp

1406

I wonder if this shouldn't be checking MFI.getRestoreBlock() instead/along with isReturnBlock(). IIUC CSRs are live after the epilogue, so I assume checking for isReturnBlock() is always safer.

[MatzeB]

lib/CodeGen/MachineBasicBlock.cpp

1406

No, we really should encode liveness with the livein lists and not add special cases. The problem is just that with the return block you have no successors where you could actually look at the live-in lists (and you already see here how complicted things become once you start special casing). For RestoreBlock != ReturnBlock we should have correct live-in lists so no special handling necessary here.

[thegameg]

lib/CodeGen/MachineBasicBlock.cpp

1406

I understand it better now, thanks for the explanation.

[arsenm]

lib/CodeGen/MachineBasicBlock.cpp

1406

I'm not sure what you're saying. If isReturnBlock is safer, then why check getRestoreBlock?

I also don't love making this change blind. Maybe this should just assert if the register is a CSR?

[kparzysz]

lib/CodeGen/MachineBasicBlock.cpp

1412–1418

Callee-saved registers are always live out of the return block, regardless of whether they have actually been saved or not.

[MatzeB]

lib/CodeGen/MachineBasicBlock.cpp

1406

The question was just whether we also need special handling for the restore block which may be different from the return block when shrink wrapping is used.

However I would say the question is no, special handling is only necessary for the return block. So the code can stay as proposed here.

1412–1418

Callee-saved registers are always live out of the return block, regardless of whether they have actually been saved or not.

Reading LivePhysRegs it seems to me that when a register is part of MachineFrameInfo::getCalleeSavedInfo() but with CSI.isRestored() == false then it is not actually live out of the return block (used to model ARMs link register which today is saved/restored via callee save mechanisms but still not live-out of the return block as it is copied to PC)

[arsenm]

lib/CodeGen/MachineBasicBlock.cpp

1406

So should I just go ahead and commit this? Is there a way to use getImplicitSPRUseForDPRUse to test this on ARM?

lib/CodeGen/MachineBasicBlock.cpp

 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/CodeGen/MachineBasicBlock.h"
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/CodeGen/LiveIntervals.h"
 #include "llvm/CodeGen/LiveVariables.h"
 #include "llvm/CodeGen/MachineDominators.h"
+#include "llvm/CodeGen/MachineFrameInfo.h"
 #include "llvm/CodeGen/MachineFunction.h"
 #include "llvm/CodeGen/MachineInstrBuilder.h"
 #include "llvm/CodeGen/MachineLoopInfo.h"
 #include "llvm/CodeGen/MachineRegisterInfo.h"
 #include "llvm/CodeGen/SlotIndexes.h"
 #include "llvm/CodeGen/TargetInstrInfo.h"
 #include "llvm/CodeGen/TargetRegisterInfo.h"
 #include "llvm/CodeGen/TargetSubtargetInfo.h"
for (++I; I != end() && N > 0; ++I) {
       if (Info.FullyDefined || Info.Clobbered)
         return LQR_Dead;
     }
   }
 
   // If we reached the end, it is safe to clobber Reg at the end of a block of
   // no successor has it live in.
   if (I == end()) {
+    const MachineFunction &MF = *getParent();
+    const MachineFrameInfo &MFI = MF.getFrameInfo();
+    if (isReturnBlock() && MFI.isCalleeSavedInfoValid()) {
+      const MachineRegisterInfo &MRI = MF.getRegInfo();
+      for (const MCPhysReg *CSR = MRI.getCalleeSavedRegs(); CSR && *CSR; ++CSR) {
+        if (*CSR == Reg)
+          return LQR_Live;
+      }
+
+      for (const CalleeSavedInfo &Info : MFI.getCalleeSavedInfo()) {
+        if (Info.isRestored() && Info.getReg() == Reg)
+          return LQR_Live;
+      }
+    }
+
     for (MachineBasicBlock *S : successors()) {
       for (MCSubRegIterator SubReg(Reg, TRI, /*IncludeSelf*/true);
            SubReg.isValid(); ++SubReg) {
         if (S->isLiveIn(*SubReg))
           return LQR_Live;
       }
     }