[PATCH] D33003: Add callee-saved registers as implicit uses in return instructions

[Matthias Braun via Phabricator via llvm-commits]

MatzeB added a comment.

As far as I understand it the main trick here is introducing the `getSavedRegisters()`/`getRestoredRegisters()` callbacks, because as it turns out the CalleeSavedInfo is not flexible enough to express what actually happens in practice (as you explained above exceptions and EH landing pads).

I found the code in `updateLiveness()` perfectly esp. compared to the new one that visited the blocks to fill a bitvector and then does something with every set bit, no idea why the step through the bitvector is necessary. Should be easy enough to do this while working like the original code. There are some good updates there like using a bitvector for the visited set or block numbers for the worklist. But factoring this out into the getAllPaths() function feels cumbersome: It requires you to first transform the list of save blocks/restore blocks which is already a nice list into a bitset just so the bitset is iterated again and put into the worklist. Similar with the results: You already visit the relevant blocks but instead of updating the liveins lists you set a bit in the bitset just so that caller has to iterate the bitset again to do something for each visited block.

If this patch works then we should be able to simplify the code in `LivePhysRegs::addLiveOutsNoPristines()` by removing all special handling for return blocks and `LivePhysRegs::addLiveOuts()` to simply add the pristine registers in the return block case.



================
Comment at: lib/CodeGen/PrologEpilogInserter.cpp:983
+
+  SmallVector<unsigned,4> Regs;
+  for (MachineBasicBlock *B : SaveBlocks) {
----------------
Maybe increasing this to 16 or 32; I'd expect more than 4 CSRs to be common.


================
Comment at: lib/CodeGen/PrologEpilogInserter.cpp:976-982
+  for (MachineBasicBlock &B : Fn)
+    if (B.isReturnBlock())
+      Returns[B.getNumber()] = true;
+  Entries[Fn.front().getNumber()] = true;
+  for (MachineBasicBlock &B : Fn)
+    if (B.isEHFuncletEntry())
+      Entries[B.getNumber()] = true;
----------------
- Those two loops should be merged.
- I guess it's customary to call block variables `MBB` in CodeGen.
- On a general note I wish that we could make better assumptions about EH landing pads, but I understand why you did it this way and it's fine for this patch.


================
Comment at: lib/CodeGen/PrologEpilogInserter.cpp:984
+
+  SmallVector<unsigned,4> Regs, PristRegs;
+  for (int x = Pristine.find_first(); x >= 0; x = Pristine.find_next(x))
----------------
Could use `MCPhysReg` instead of `unsigned`.


================
Comment at: lib/CodeGen/PrologEpilogInserter.cpp:985-986
+  SmallVector<unsigned,4> Regs, PristRegs;
+  for (int x = Pristine.find_first(); x >= 0; x = Pristine.find_next(x))
+    PristRegs.push_back(x);
+
----------------
Francis landed a patch recently that finally allows `for(unsigned PRegs : Pristine.set_bits())`. Similar in a few other places here (though as noted in the general comments, it feels like too many unnecessary intermediate bitsets to me).


================
Comment at: lib/CodeGen/PrologEpilogInserter.cpp:991
+    TFI.getSavedRegisters(Regs, *B, CSI, TRI);
+    Regs.insert(Regs.end(), PristRegs.begin(), PristRegs.end());
+    if (!Regs.empty())
----------------
Sorry for the confusion about the pristine registers.
Handling them like this is wrong: You need to push them into every single livein list in the function, not just to the ones betweeen entry/save and restore/return.

I think we better leave that to another patch then (after measuring the impact on compiletime/codesize).


================
Comment at: lib/CodeGen/PrologEpilogInserter.cpp:1009
+  BitVector &Down = Paths;     // Blocks reachable from Starts going downwards.
+  SetVector<unsigned> Worklist;
+
----------------
A normal list should do here, as you already habe visited sets around. You only add stuff to the worklist that is not in the visited set yet. That allows you to use a simple Vector again.


================
Comment at: lib/Target/ARM/ARMLoadStoreOptimizer.cpp:1923-1929
+      for (MachineOperand &Op : MBBI->operands()) {
+        if (!Op.isReg() || !Op.isImplicit())
+          continue;
+        if (Op.isUse() && Prev->modifiesRegister(Op.getReg(), TRI))
+          continue;
+        NewMI.add(Op);
+      }
----------------
Maybe factor out the duplicated code.


Repository:
  rL LLVM

https://reviews.llvm.org/D33003