[MachineCombiner] Update iterator while deleting instructions

[DarshanRamakant]

Summary

This change will fix the accidental deletion of the instruction
which is pointed by the instruction iterator of the current MBB.

[craig.topper]

Test case?

[DarshanRamakant]

In D119070#3299166, @craig.topper wrote:

Test case?

In D119070#3299166, @craig.topper wrote:

Test case?

I did not find any existing test case which specifically tests the "insertDeleteInstructions()" function. Also, I did not find MIR test cases with many of the previous commits. Please provide me with some pointers on testing this specific change. I found this issue when experimenting with this transform for my company-specific backend.

[arsenm]

In D119070#3299167, @DarshanRamakant wrote:

I did not find any existing test case which specifically tests the "insertDeleteInstructions()" function. Also, I did not find MIR test cases with many of the previous commits. Please provide me with some pointers on testing this specific change. I found this issue when experimenting with this transform for my company-specific backend.

I would hope there were at least some IR tests in previous commits touching this pass. Perhaps you can use -stop-after to get a MIR starting point on one of them, and manually modify the MIR until it breaks?

[DarshanRamakant]

In D119070#3299315, @arsenm wrote:

In D119070#3299167, @DarshanRamakant wrote:

I did not find any existing test case which specifically tests the "insertDeleteInstructions()" function. Also, I did not find MIR test cases with many of the previous commits. Please provide me with some pointers on testing this specific change. I found this issue when experimenting with this transform for my company-specific backend.

I would hope there were at least some IR tests in previous commits touching this pass. Perhaps you can use -stop-after to get a MIR starting point on one of them, and manually modify the MIR until it breaks?

Thanks for the suggestion. Let me try that approach.

[DarshanRamakant]

In D119070#3299948, @DarshanRamakant wrote:

In D119070#3299315, @arsenm wrote:

In D119070#3299167, @DarshanRamakant wrote:

I did not find any existing test case which specifically tests the "insertDeleteInstructions()" function. Also, I did not find MIR test cases with many of the previous commits. Please provide me with some pointers on testing this specific change. I found this issue when experimenting with this transform for my company-specific backend.

I would hope there were at least some IR tests in previous commits touching this pass. Perhaps you can use -stop-after to get a MIR starting point on one of them, and manually modify the MIR until it breaks?

Thanks for the suggestion. Let me try that approach.

I have tried to add a test case by tweaking existing ll tests. But the existing overloads of this transform will not hit this case since they always delete instructions that are previous to "Root". But nobody is stopping clients of this transform to mark currently pointed instruction as a to be deleted instruction. It has become very tricky to hit this case with a test. Please let me know how to proceed further.

[arsenm]

llvm/lib/CodeGen/MachineCombiner.cpp

489

Typo cuurent

492

I've never looked at this pass before, but this looks like a strange structure. Why can't this pre-increment the iterator before making changes to the function?

[DarshanRamakant]

llvm/lib/CodeGen/MachineCombiner.cpp

489

Thanks will fix it.

492

BlockItr will be pointing to the next instruction to be processed. If the instruction marked for delete happens to be exactly that, then line 570 will become a dangling pointer and will have an undefined behavior.

llvm/lib/CodeGen/MachineCombiner.cpp

 /// \param DelInstrs instruction to delete from \p MBB
 /// \param MinInstr is a pointer to the machine trace information
 /// \param RegUnits set of live registers, needed to compute instruction depths
 /// \param TII is target instruction info, used to call target hook
 /// \param Pattern is used to call target hook finalizeInsInstrs
 /// \param IncrementalUpdate if true, compute instruction depths incrementally,
 ///                          otherwise invalidate the trace
 static void insertDeleteInstructions(MachineBasicBlock *MBB, MachineInstr &MI,
+                                     MachineBasicBlock::iterator &BlockItr,
                                      SmallVector<MachineInstr *, 16> InsInstrs,
                                      SmallVector<MachineInstr *, 16> DelInstrs,
                                      MachineTraceMetrics::Ensemble *MinInstr,
                                      SparseSet<LiveRegUnit> &RegUnits,
                                      const TargetInstrInfo *TII,
                                      MachineCombinerPattern Pattern,
                                      bool IncrementalUpdate) {
   // If we want to fix up some placeholder for some target, do it now.
   // We need this because in genAlternativeCodeSequence, we have not decided the
   // better pattern InsInstrs or DelInstrs, so we don't want generate some
   // sideeffect to the function. For example we need to delay the constant pool
   // entry creation here after InsInstrs is selected as better pattern.
   // Otherwise the constant pool entry created for InsInstrs will not be deleted
   // even if InsInstrs is not the better pattern.
   TII->finalizeInsInstrs(MI, Pattern, InsInstrs);
 
   for (auto *InstrPtr : InsInstrs)
     MBB->insert((MachineBasicBlock::iterator)&MI, InstrPtr);
 
   for (auto *InstrPtr : DelInstrs) {
+    // If the instructions to be deleted is pointed by cuurent iterator
+    // update the iterator.
+    if (InstrPtr == &*BlockItr) {
+      BlockItr++;
+    }
     InstrPtr->eraseFromParent();
     // Erase all LiveRegs defined by the removed instruction
     for (auto I = RegUnits.begin(); I != RegUnits.end(); ) {
       if (I->MI == InstrPtr)
         I = RegUnits.erase(I);
       else
         I++;
     }
for (auto P : Patterns) {
               CombinerObjective::MustReduceRegisterPressure) {
         if (MBB->size() > inc_threshold) {
           // Use incremental depth updates for basic blocks above threshold
           IncrementalUpdate = true;
           LastUpdate = BlockIter;
         }
         if (reduceRegisterPressure(MI, MBB, InsInstrs, DelInstrs, P)) {
           // Replace DelInstrs with InsInstrs.
-          insertDeleteInstructions(MBB, MI, InsInstrs, DelInstrs, MinInstr,
-                                   RegUnits, TII, P, IncrementalUpdate);
+          insertDeleteInstructions(MBB, MI, BlockIter, InsInstrs, DelInstrs,
+                                   MinInstr, RegUnits, TII, P,
+                                   IncrementalUpdate);
           Changed |= true;
 
           // Go back to previous instruction as it may have ILP reassociation
           // opportunity.
           BlockIter--;
           break;
         }
       }
 
       // Substitute when we optimize for codesize and the new sequence has
       // fewer instructions OR
       // the new sequence neither lengthens the critical path nor increases
       // resource pressure.
       if (SubstituteAlways ||
           doSubstitute(NewInstCount, OldInstCount, OptForSize)) {
-        insertDeleteInstructions(MBB, MI, InsInstrs, DelInstrs, MinInstr,
-                                 RegUnits, TII, P, IncrementalUpdate);
+        insertDeleteInstructions(MBB, MI, BlockIter, InsInstrs, DelInstrs,
+                                 MinInstr, RegUnits, TII, P, IncrementalUpdate);
         // Eagerly stop after the first pattern fires.
         Changed = true;
         break;
       } else {
         // For big basic blocks, we only compute the full trace the first time
         // we hit this. We do not invalidate the trace, but instead update the
         // instruction depths incrementally.
         // 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) {
             // Use incremental depth updates for basic blocks above treshold
             IncrementalUpdate = true;
             LastUpdate = BlockIter;
           }
 
-          insertDeleteInstructions(MBB, MI, InsInstrs, DelInstrs, MinInstr,
-                                   RegUnits, TII, P, IncrementalUpdate);
+          insertDeleteInstructions(MBB, MI, BlockIter, InsInstrs, DelInstrs,
+                                   MinInstr, RegUnits, TII, P,
+                                   IncrementalUpdate);
 
           // Eagerly stop after the first pattern fires.
           Changed = true;
           break;
         }
         // Cleanup instructions of the alternative code sequence. There is no
         // use for them.
         MachineFunction *MF = MBB->getParent();