[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:377
+    auto End = ++MI.getIterator();
+    for (; LastUpdate != End; LastUpdate++) {
+      MinInstr->updateDepth(MBB, *LastUpdate, RegUnits);
----------------
Gerolf wrote:
> Gerolf wrote:
> > The assumption is that LastUpdate and End are never Null. Would it make sense to document this with an assertion.
> The loops under IncrementalUpdate  in this function could be wrapped in a function (with start and end iterators as parameters).
I think I am missing something about the assertion, but neither LatestUpdate nor End are pointers.

 I've added an updateDepths function


================
Comment at: lib/CodeGen/MachineCombiner.cpp:389
     InstrPtr->eraseFromParentAndMarkDBGValuesForRemoval();
-  ++NumInstCombined;
-  Traces->invalidate(MBB);
-  Traces->verifyAnalysis();
+    // Erase all LiveRegs defined by the removed instruction
+    for (auto I = RegUnits.begin(); I != RegUnits.end(); ) {
----------------
Gerolf wrote:
> Would it make sense for the RegUnit update be part of eraseFromParent...(),? E.g. have a bool parameter for that function indicating it?
I think updating RegUnits is only relevant for the incremental updates here and I am not sure if it is worth making Instruction::eraseFromParent... more compilcated.


https://reviews.llvm.org/D36619