[MachineCombiner] Fix initialisation of LastUpdate for incremental update.

[paulwalker-arm]

Summary

Fixes a bogus iterator resulting from the removal of a block's first instruction at the point that incremental update is enabled.

Patch by Paul Walker.

[fhahn]

LGTM, thanks for fixing that Paul! Unless there are any additional concerns, I can commit your change tomorrow.

Just to re-iterate, the test/CodeGen/AArch64/machine-combiner.mir was added because in the IR version of the test case, the bug would not be triggered, as the DAGCombiner would kick in before the MachineCombiner. We might want to convert the existing machine-combiner.ll test case to a MIR case, to make sure this test actually exercises the MachineCombiner.

lib/CodeGen/MachineCombiner.cpp

417

just out of curiosity, would auto auto BlockIter = MBB->begin(), LastUpdate; work?

[aemerson]

lib/CodeGen/MachineCombiner.cpp

417

At this point just use the type explicitly?

[MatzeB]

Please take a look at http://llvm.org/docs/MIRLangRef.html#simplifying-mir-files and simplify the test.

[paulwalker-arm]

As requested I've simplified the patch's mir test.

[paulwalker-arm]

lib/CodeGen/MachineCombiner.cpp

417

The first suggestion is not valid C++. Given the surrounding code, decltype seems like the correct choice. I can make the types explicit, but then why allow auto in the first place?

[aemerson]

lib/CodeGen/MachineCombiner.cpp

417

No objection from me, just think: MachineBasicBlock::iterator BlockIter = MBB->begin(), LastUpdate; is nicer in this case.

Florian I think you're good to commit.

[Gerolf]

Thanks for working on this. LGTM!

-Gerolf

[fhahn]

Thanks everyone!

lib/CodeGen/MachineCombiner.cpp

 /// instructions  when this neither lengthens the critical path nor increases
 /// resource pressure. When optimizing for codesize always combine when the new
 /// sequence is shorter.
 bool MachineCombiner::combineInstructions(MachineBasicBlock *MBB) {
   bool Changed = false;
   DEBUG(dbgs() << "Combining MBB " << MBB->getName() << "\n");
 
   bool IncrementalUpdate = false;
   auto BlockIter = MBB->begin();
-  auto LastUpdate = BlockIter;
+  decltype(BlockIter) LastUpdate;
   // Check if the block is in a loop.
   const MachineLoop *ML = MLI->getLoopFor(MBB);
   if (!MinInstr)
     MinInstr = Traces->getEnsemble(MachineTraceMetrics::TS_MinInstrCount);
 
   SparseSet<LiveRegUnit> RegUnits;
   RegUnits.setUniverse(TRI->getNumRegUnits());
 
for (auto P : Patterns) {
         // NOTE: Only the instruction depths up to MI are accurate. All other
         // trace information is not updated.
         MachineTraceMetrics::Trace BlockTrace = MinInstr->getTrace(MBB);
         Traces->verifyAnalysis();
         if (improvesCriticalPathLen(MBB, &MI, BlockTrace, InsInstrs, DelInstrs,
                                     InstrIdxForVirtReg, P,
                                     !IncrementalUpdate) &&
             preservesResourceLen(MBB, BlockTrace, InsInstrs, DelInstrs)) {
-          if (MBB->size() > inc_threshold)
+          if (MBB->size() > inc_threshold) {
             // Use incremental depth updates for basic blocks above treshold
             IncrementalUpdate = true;
+            LastUpdate = BlockIter;
+          }
 
           insertDeleteInstructions(MBB, MI, InsInstrs, DelInstrs, MinInstr,
                                    RegUnits, IncrementalUpdate);
 
           // Eagerly stop after the first pattern fires.
           Changed = true;
           break;
         }

test/CodeGen/AArch64/machine-combiner.mir

+# RUN: llc -mtriple=aarch64-none-linux-gnu -mcpu=cortex-a57 -enable-unsafe-fp-math \
+# RUN:     -run-pass machine-combiner -machine-combiner-inc-threshold=0 \
+# RUN:     -verify-machineinstrs  -o - %s | FileCheck %s
+---
+# Test incremental depth updates succeed when triggered after the removal of
+# the first instruction in a basic block.
+
+# CHECK-LABEL: name: inc_update_iterator_test
+name:            inc_update_iterator_test
+registers:
+  - { id: 0, class: fpr64 }
+  - { id: 1, class: gpr32 }
+  - { id: 2, class: gpr32 }
+  - { id: 3, class: gpr32 }
+  - { id: 4, class: gpr32 }
+  - { id: 5, class: gpr32 }
+  - { id: 6, class: gpr32 }
+  - { id: 7, class: fpr64 }
+  - { id: 8, class: fpr64 }
+  - { id: 9, class: fpr64 }
+body:             |
+  bb.0:
+    successors: %bb.1, %bb.2
+
+    %3 = COPY %w2
+    %2 = COPY %w1
+    %1 = COPY %w0
+    %0 = COPY %d0
+    %4 = SUBSWrr %1, %2, implicit-def %nzcv
+    Bcc 13, %bb.2, implicit %nzcv
+    B %bb.1
+
+  bb.1:
+    ; CHECK: MADDWrrr %1, %2, %3
+    %5 = MADDWrrr %1, %2, %wzr
+    %6 = ADDWrr %3, killed %5
+    %7 = SCVTFUWDri killed %6
+    ; CHECK: FMADDDrrr %7, %7, %0
+    %8 = FMULDrr %7, %7
+    %9 = FADDDrr %0, killed %8
+    %d0 = COPY %9
+    RET_ReallyLR implicit %d0
+
+  bb.2:
+    %d0 = COPY %0
+    RET_ReallyLR implicit %d0
+
+...