[PATCH] D51474: Consider CSRs in computeRegisterLiveness

[Matthias Braun via Phabricator via llvm-commits]

MatzeB added inline comments.


================
Comment at: lib/CodeGen/MachineBasicBlock.cpp:1412-1418
+      const MachineFrameInfo &MFI = MF.getFrameInfo();
+      if (MFI.isCalleeSavedInfoValid()) {
+        for (const CalleeSavedInfo &Info : MFI.getCalleeSavedInfo()) {
+          if (Info.isRestored() && Info.getReg() == Reg)
+            return LQR_Live;
+        }
+      }
----------------
kparzysz wrote:
> MatzeB wrote:
> > MatzeB wrote:
> > > - 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.
> > > 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...
> Callee-saved registers are always live out of the return block, regardless of whether they have actually been saved or not.
> 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)


================
Comment at: lib/CodeGen/MachineBasicBlock.cpp:1406
+    const MachineFrameInfo &MFI = MF.getFrameInfo();
+    if (isReturnBlock() && MFI.isCalleeSavedInfoValid()) {
+      const MachineRegisterInfo &MRI = MF.getRegInfo();
----------------
arsenm wrote:
> thegameg wrote:
> > MatzeB wrote:
> > > thegameg wrote:
> > > > 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.
> > > 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.
> > I understand it better now, thanks for the explanation.
> 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?
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.


https://reviews.llvm.org/D51474