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

[Gerolf Hoflehner via Phabricator via llvm-commits]

Gerolf added a comment.

Thank you for sharing your numbers. I was hoping you had that show-off case.
I just have a few minor comments about readability and asserts. I still need bit more time to convince myself that all the pieces fit together. I was wondering if you could add a test that dumps the depths w/ and w/o the incremental updates (compile w/ full comination + dumps depths, compile with a threashold of 1 + dump depths, diff the dump)?

Thanks
Gerolf



================
Comment at: lib/CodeGen/MachineCombiner.cpp:366
 
 static void insertDeleteInstructions(MachineBasicBlock *MBB, MachineInstr &MI,
                                      SmallVector<MachineInstr *, 16> InsInstrs,
----------------
Please add comments what the parameters mean. More parameters make it harder to read now.


================
Comment at: lib/CodeGen/MachineCombiner.cpp:377
+    auto End = ++MI.getIterator();
+    for (; LastUpdate != End; LastUpdate++) {
+      MinInstr->updateDepth(MBB, *LastUpdate, RegUnits);
----------------
The assumption is that LastUpdate and End are never Null. Would it make sense to document this with an assertion.


================
Comment at: lib/CodeGen/MachineCombiner.cpp:377
+    auto End = ++MI.getIterator();
+    for (; LastUpdate != End; LastUpdate++) {
+      MinInstr->updateDepth(MBB, *LastUpdate, RegUnits);
----------------
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).


================
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(); ) {
----------------
Would it make sense for the RegUnit update be part of eraseFromParent...(),? E.g. have a bool parameter for that function indicating it?


https://reviews.llvm.org/D36619