Page MenuHomePhabricator

Consider CSRs in computeRegisterLiveness
Needs ReviewPublic

Authored by arsenm on Aug 29 2018, 11:12 PM.

Details

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.

Diff Detail

Event Timeline

arsenm created this revision.Aug 29 2018, 11:12 PM

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 added inline comments.Aug 30 2018, 5:00 PM
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 updated this revision to Diff 163475.Aug 31 2018, 1:22 AM

Only check if MFI.isCalleeSavedInfoValid

thegameg added inline comments.
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 added inline comments.Sep 4 2018, 1:15 PM
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 added inline comments.Sep 4 2018, 1:24 PM
lib/CodeGen/MachineBasicBlock.cpp
1406

I understand it better now, thanks for the explanation.

arsenm added inline comments.Sep 5 2018, 8:01 AM
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 added inline comments.
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 added inline comments.Sep 5 2018, 9:09 AM
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 added inline comments.Sep 10 2018, 9:56 PM
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?