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

[Matthias Braun via Phabricator via llvm-commits]

MatzeB accepted this revision.
MatzeB added a comment.
This revision is now accepted and ready to land.

Accepting this now as it looks reasonably safe and fast to me (nitpicks below).

It is a conservative/simple heuristic but given the fact that we lack other means of global code motion (in the sense of inter-basic block) in CodeGen today this is fine.

Please wait for some of the other reviewers which showed interest here to accept before committing.



================
Comment at: lib/CodeGen/LiveRangeShrink.cpp:10-12
+// This pass moves instructions close to the definition of its operands to
+// shrink live range of the def instruction. The code motion is limited within
+// the basic block.
----------------
MatzeB wrote:
> Use `/// \file`, see http://llvm.org/docs/CodingStandards.html#file-headers
> 
> It would also be a good idea to add a sentence describing the rules/heuristic that decide what instructions get moved.
Needs three slashes to be recognized as doxygen. (I know this is wrong in a lot of existing code, but we should get it right in new code)


================
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());
+        }
----------------
danielcdh wrote:
> 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.
I'm not sure why this would work. Maybe you should double check by compiling the llvm-testsuite (or some other set of programs) with a debug compiler and `-g`...


================
Comment at: lib/CodeGen/LiveRangeShrink.cpp:112
+    // last use when moving up.
+    DenseMap<unsigned, unsigned> UseMap;
+
----------------
Maybe move UseMap outside the loop and `clear()` it each iteration instead so there is a chance that memory allocations can be reused.


https://reviews.llvm.org/D32563