[PATCH] D36619: [MachineCombiner] Update instruction depths incrementally for large BBs.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: lib/CodeGen/MachineCombiner.cpp:280
     return NewRootDepth < RootDepth;
 
   unsigned NewRootLatency = getLatency(Root, NewRoot, BlockTrace);
----------------
Gerolf wrote:
> What happened to the comment?
I've added the comment again. Removing the comment was a left-over from an earlier iteration of the patch.


================
Comment at: lib/CodeGen/MachineCombiner.cpp:487
       } else {
-        // Calculating the trace metrics may be expensive,
-        // so only do this when necessary.
+        // For big basic blocks, Wwe only compute the full trace the first time
+        // we hit this the first  time. We do not invalidate the trace, but
----------------
Gerolf wrote:
> Typos (eg. Wwe -> We) and dups ('the first time'). What does 'relatively accurate' mean?
I am not sure how to put that. updateDepth() uses the live register info in `RegUnits` to track dependencies of physical registers.  In the following case, the depths could be wrong (R1 being a physical register):

R1 <- I0 ....
R2 <- I1 R1
R1 <- I2 ...
..    <- I3 R2

If we replace I1 and I2 with `... <- I4 R1`, when we are updating the depth of I4, RegUnits would contain something like (R1 defined by I2), and it would use that dependency for the depth calculation. But when the MachineCombiner is run, combined instructions should still use virtual registers. At least for the AArch64, it seems like physical registers are used only for argument passing at this stage.



https://reviews.llvm.org/D36619