[PATCH] D32563: Add LiveRangeShrink pass to shrink live range within BB.

[Dehao Chen via Phabricator via llvm-commits]

danielcdh marked 21 inline comments as done.
danielcdh added a comment.

Thanks for the review.



================
Comment at: lib/CodeGen/LiveRangeShrink.cpp:64
+    return New;
+  int OrderOld = M.find(Old)->second;
+  int OrderNew = M.find(New)->second;
----------------
MatzeB wrote:
> `M[Old]`?
M is const map, thus cannot use M[Old].


================
Comment at: lib/CodeGen/LiveRangeShrink.cpp:84
+  int i = 0;
+  for (MachineInstr &I : make_range(Start, Start->getParent()->instr_end())) {
+    M[&I] = i++;
----------------
MatzeB wrote:
> Why not pass a `MachineBasicBlock &MBB` as parameter to the function instead of the iterator and doing the `Start->getParent()->instr_end()` dance? You could have just used `for (MachineInstr &I : MBB) {}` in that case.
Because Start may not be MBB.begin(), but could also be instruction that has side effect.

The reason I used Start->getParent()->instr_end() instead of passing in MBB and use MBB.instr_end() is because I want to pass one less parameter to make the function clearer.


================
Comment at: lib/CodeGen/LiveRangeShrink.cpp:93-94
+// Scan instructions following MI and collect any matching DBG_VALUEs.
+// FIXME: this is copied from MachineSink.cpp, need to refactor them into
+//        utility function.
+void collectDebugValues(MachineInstr &MI,
----------------
MatzeB wrote:
> Then why not do so in this patch.
Code updated to inline the logic into caller to make the logic clearer.


================
Comment at: lib/CodeGen/LiveRangeShrink.cpp:125
+    InstOrderMap IOM;
+    bool SawStore = BuildInstOrderMap(MBB.instr_begin(), IOM);
+
----------------
MatzeB wrote:
> Collecting the SawStore value is unnecessary. The only user appears to be `MachineInstr::isSafeToMove()` below which writes(!) the variable.
SawStore is both input and output of MachineInstr::isSafeToMove().


================
Comment at: lib/CodeGen/LiveRangeShrink.cpp:211-215
+        MBB.splice(I, &MBB, MI->getIterator());
+        for (MachineInstr *DI : DbgValuesToMove) {
+          IOM[DI] = IOM[MI];
+          MBB.splice(I, &MBB, DI->getIterator());
+        }
----------------
MatzeB wrote:
> There's a variant of splice that can move a sequence of instructions which should work nicely here.
> 
> It is also not clear to me if moving the debug instructions is legal here. They are not considered when checking how early an instruction can move so you are possibly moving the debug instruction above a point where the value its operand is defined.
Done

For the Debug Value, it should be legal as the same logic is used by MachineLICM.


https://reviews.llvm.org/D32563