[globalisel] Improve legalizer DEBUG_ONLY output.

[dsanders]

Summary

This patch changes the way we decide which instructions to emit in the
DEBUG_ONLY output. It now emits all instructions that replace the work
list item regardless of their creation method (previously instructions
created with BuildMI() were not emitted) and it no longer requires the
new instructions to be added to the work list.

[aditya_nandakumar]

Just to clarify, this wouldn't work if the target decided to insert new instructions before PrevMII (or even at the beginning for e.g.). I'm guessing if the target does that, its not a big deal if we don't print it.

[dsanders]

In D32744#745035, @aditya_nandakumar wrote:

Just to clarify, this wouldn't work if the target decided to insert new instructions before PrevMII (or even at the beginning for e.g.). I'm guessing if the target does that, its not a big deal if we don't print it.

That's right. It's my understanding that targets are supposed to substitute instructions in place, similar to the legalizer in SelectionDAG. If a target does something else it will need to add it's own DEBUG()

[qcolombet]

Hi Daniel,

Although the patch makes sense, I am not sure we can expect that PrintFrom and PrintTo are going to be valid at this point. For instance, CurrMI could fold PrintFrom with a custom legalizer action.

I don't have an alternative to offer though :(

Cheers,
-Quentin

[dsanders]

I believe PrintTo will be valid since it's occurs after the instruction we're working on. If we're folding multiple instructions into one we'll be following the operands backwards, away from PrintTo.

For PrintFrom: At the moment, we only erase the current instruction and we leave operands we fold into it for the trivially-dead check in case there's another user of one of the defs for that operand. If that will continue to be the case, then PrintFrom is safe.

[qcolombet]

It does not sound to me like something we can ensure. I wouldn't pursue in that direction IMHO.

[aditya_nandakumar]

Is this still relevant?

[dsanders]

Tracing the legalizer is still something we need to improve on but this patch isn't suitable given that we aren't restricting the insertion point.

lib/CodeGen/GlobalISel/Legalizer.cpp

for (auto MI = MBB.begin(); MI != MBB.end(); MI = NextMI) {
       // Get the next Instruction before we try to legalize, because there's a
       // good chance MI will be deleted.
       NextMI = std::next(MI);
 
       // Only legalize pre-isel generic instructions: others don't have types
       // and are assumed to be legal.
       if (!isPreISelGenericOpcode(MI->getOpcode()))
         continue;
-      unsigned NumNewInsns = 0;
       SmallVector<MachineInstr *, 4> WorkList;
       Helper.MIRBuilder.recordInsertions([&](MachineInstr *MI) {
-        ++NumNewInsns;
         WorkList.push_back(MI);
       });
       WorkList.push_back(&*MI);
 
       bool Changed = false;
       LegalizerHelper::LegalizeResult Res;
       unsigned Idx = 0;
       do {
-        Res = Helper.legalizeInstrStep(*WorkList[Idx]);
+        MachineInstr &CurrMI = *WorkList[Idx];
+#ifndef NDEBUG
+        // Record the instructions either side of the current instruction so we
+        // can iterate over its replacements (even after CurrMI has been erased).
+        // PrintFrom is set to MBB.end() if there is no instruction before CurrMI.
+        MachineBasicBlock::iterator PrintFrom = MBB.end();
+        if (CurrMI != MBB.begin()) {
+          PrintFrom = CurrMI;
+          --PrintFrom;
+        }
+        MachineBasicBlock::iterator PrintTo =
+            std::next(MachineBasicBlock::iterator(CurrMI));
+#endif // NDEBUG
+
+        Res = Helper.legalizeInstrStep(CurrMI);
         // Error out if we couldn't legalize this instruction. We may want to
         // fall back to DAG ISel instead in the future.
         if (Res == LegalizerHelper::UnableToLegalize) {
           Helper.MIRBuilder.stopRecordingInsertions();
           if (Res == LegalizerHelper::UnableToLegalize) {
             reportGISelFailure(MF, TPC, MORE, "gisel-legalize",
-                               "unable to legalize instruction",
-                               *WorkList[Idx]);
+                               "unable to legalize instruction", CurrMI);
             return false;
           }
         }
         Changed |= Res == LegalizerHelper::Legalized;
-        ++Idx;
 
 #ifndef NDEBUG
-        if (NumNewInsns) {
-          DEBUG(dbgs() << ".. .. Emitted " << NumNewInsns << " insns\n");
-          for (auto I = WorkList.end() - NumNewInsns, E = WorkList.end();
-               I != E; ++I)
-            DEBUG(dbgs() << ".. .. New MI: "; (*I)->print(dbgs()));
-          NumNewInsns = 0;
-        }
-#endif
+        // Now that the current instruction has been erased (if it was going to
+        // be) advancing PrintFrom will select it again if we didn't erase it,
+        // select PrintTo if we erased it but emitted zero instructions, or the
+        // first newly created instruction.
+        if (PrintFrom == MBB.end())
+          PrintFrom = MBB.begin();
+        else
+          ++PrintFrom;
+
+        for (auto I = PrintFrom, E = PrintTo; I != E; ++I)
+          DEBUG(dbgs() << ".. .. MI: "; I->print(dbgs()));
+#endif // NDEBUG
+
+        ++Idx;
       } while (Idx < WorkList.size());
 
       Helper.MIRBuilder.stopRecordingInsertions();
     }
   }
 
   MachineRegisterInfo &MRI = MF.getRegInfo();
   const TargetInstrInfo &TII = *MF.getSubtarget().getInstrInfo();