[PATCH] D36160: Liveness issues in tail merging, and the ARM::LR saved-but-not-restored

[Matthias Braun via Phabricator via llvm-commits]

MatzeB added a comment.

Thanks for working on this!

1. The BranchFolding changes: I've got some issue with the current approach, see below.

2. Adding the Restored bit to CSI:

- Very interesting observations!
- Can we split this change into a separate patch?
- I think the proposed change is fine as it fixes a problem with the system we have today (under the condition that you add more comments).

My long/in an ideal world:
I find the code in `LivePhysRegs::addLiveOuts()` and `LiveRegUntis::addLiveOuts()` that queries CSI on return blocks is accidental complexity that should not be there. The CSI list should be an implementation detail of the prolog epilogue inserter and disappear after that pass is done. We should be able to model things perfectly well by adding implicit uses to the return instructions; It should be equally natural to not add the implicit use of LR to `pop {pc}` then.



================
Comment at: include/llvm/CodeGen/MachineFrameInfo.h:31-45
 class CalleeSavedInfo {
   unsigned Reg;
   int FrameIdx;
+  bool Restored;
 
 public:
   explicit CalleeSavedInfo(unsigned R, int FI = 0)
----------------
The discussion here clearly shows that `Restored` is an unintuitive concept, so we need comments to explain it in detail!


================
Comment at: include/llvm/CodeGen/MachineFrameInfo.h:671
+  std::vector<CalleeSavedInfo> &getCalleeSavedInfo() { return CSInfo; }
   const std::vector<CalleeSavedInfo> &getCalleeSavedInfo() const {
     return CSInfo;
----------------
You could use `/// \copydoc getCalleeSavedInfo()` here.


================
Comment at: lib/CodeGen/BranchFolding.cpp:241-253
+void BranchFolder::computeLiveOuts(LivePhysRegs &LiveOut,
+                                   const MachineBasicBlock &MBB) {
+  LiveOut.init(*TRI);
+  LiveOut.addLiveIns(MBB);
+
+  SmallVector<std::pair<unsigned, const MachineOperand*>,8> Clobbers;
+  for (const MachineInstr &MI : MBB) {
----------------
- Why do you need this, given there is already `LivePhysRegs::addLiveOuts()`/`LivePhysRegs::addLiveOutsNoPristines()`. If there is a reason, then this needs a comment explaining it.
- We are working on deprecating kill-flags. This means introducing new uses of `stepForward()` is bad as the results will be better/worse depending on kill-flag accuracy.
- (Unrelated to this patch: I didn't realize you have to manually remove dead defs from the LiveOut set like this, I really think `stepForward()` should already do this for you and should be fixed accordingly)


Repository:
  rL LLVM

https://reviews.llvm.org/D36160