[globalisel] Enable tracing the legalizer with --debug-only=legalize-mir

[kristof.beyls]

lib/CodeGen/GlobalISel/LegalizerHelper.cpp

69–70

This patch looks pretty straightforward.
I'm just wondering why you don't just directly print out the New MI here and instead use a data structure to record it and print it out later?

[dsanders]

lib/CodeGen/GlobalISel/LegalizerHelper.cpp

69–70

I didn't give it enough thought to notice it was redundant :-). I'll make it emit it directly once I've finished updating the other patches.

[dsanders]

Refresh patch. No other changes made (see next comment).

[dsanders]

lib/CodeGen/GlobalISel/LegalizerHelper.cpp

69–70

I got around to trying to make this change and remembered why I recorded the MI and printed it out afterwards. The operands haven't been added to the instruction at the point that recordInsertions() is called so it only prints the opcode. This makes it hard to trace the changes the pass is making.

[kristof.beyls]

lib/CodeGen/GlobalISel/Legalizer.cpp

174

I think that just having an "unsigned DebugNewInstructionsIdx" or something similar could be enough to track the new MIs that are created but haven't been printed yet for debugging; no need for a separate SmallVector?
I think it makes the code slightly easier to understand, as it's then immediately clear that MI's from the WorkList are being printed, rather than having to look through code how this new data structure DebugReportList is being formed.

lib/CodeGen/GlobalISel/LegalizerHelper.cpp

69–70

Ah yes, that makes sense.

[dsanders]

SmallVector -> number of new instructions emitted.

Also fixed a small issue I missed in the refresh. Now that the debug output
code is split across two files with different DEBUG_ONLY definitions you need
--debug-only=legalize-mir,legalizer to get useful debug output. legalize-mir
isn't a pass and doesn't provide useful debug output without legalizer so I've
made them both enable their debug output when given --debug-only=legalizer.

[kristof.beyls]

lib/CodeGen/GlobalISel/Legalizer.cpp

204

Minor nitpick: maybe use auto& here to make the code slightly more readable?
Otherwise, LGTM.

[dsanders]

lib/CodeGen/GlobalISel/Legalizer.cpp

204

Thanks.

[aditya_nandakumar]

lib/CodeGen/GlobalISel/Legalizer.cpp

204

Quick question here - is it possible that one of the instructions could have been erased as it went through legalization (and we're trying to print it)?

[kristof.beyls]

lib/CodeGen/GlobalISel/Legalizer.cpp

204

I'm assuming that's not possible as the instructions being printed here are the ones added to the WorkList in this iteration, to be processed in future iterations. In other words, all of these MIs will be processed by legalizeInstrStep later on, so they shouldn't have been erased at this point.

[dsanders]

lib/CodeGen/GlobalISel/Legalizer.cpp

204

That's right. The only way it could see an erased instruction is if a single legalizer step created an instruction and erased it again. That would be wasteful and would break the work list too.

lib/CodeGen/GlobalISel/Legalizer.cpp

bool Legalizer::runOnMachineFunction(MachineFunction &MF) {
 
   // FIXME: an instruction may need more than one pass before it is legal. For
   // example on most architectures <3 x i3> is doubly-illegal. It would
   // typically proceed along a path like: <3 x i3> -> <3 x i8> -> <8 x i8>. We
   // probably want a worklist of instructions rather than naive iterate until
   // convergence for performance reasons.
   bool Changed = false;
   MachineBasicBlock::iterator NextMI;
-  for (auto &MBB : MF)
+  for (auto &MBB : MF) {
     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;
+      SmallVector<MachineInstr *, 4> DebugReportList LLVM_ATTRIBUTE_UNUSED;
       SmallVector<MachineInstr *, 4> WorkList;
-      Helper.MIRBuilder.recordInsertions(
-          [&](MachineInstr *MI) { WorkList.push_back(MI); });
+      Helper.MIRBuilder.recordInsertions([&](MachineInstr *MI) {
+        DEBUG(DebugReportList.push_back(MI));
+        WorkList.push_back(MI);
+      });
       WorkList.push_back(&*MI);
 
+      bool Changed = false;
       LegalizerHelper::LegalizeResult Res;
       unsigned Idx = 0;
       do {
         Res = Helper.legalizeInstrStep(*WorkList[Idx]);
         // Error out if we couldn't legalize this instruction. We may want to
-        // fall
-        // back to DAG ISel instead in the future.
+        // 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]);
             return false;
           }
         }
         Changed |= Res == LegalizerHelper::Legalized;
         ++Idx;
+
+#ifndef NDEBUG
+        for (const auto *MI : DebugReportList)
+          DEBUG(dbgs() << ".. .. New MI: "; MI->print(dbgs()));
+        DebugReportList.clear();
+#endif
       } while (Idx < WorkList.size());
 
       Helper.MIRBuilder.stopRecordingInsertions();
     }
+  }
 
   MachineRegisterInfo &MRI = MF.getRegInfo();
   const TargetInstrInfo &TII = *MF.getSubtarget().getInstrInfo();
   for (auto &MBB : MF) {
     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);
 
       Changed |= combineExtracts(*MI, MRI, TII);
       Changed |= combineMerges(*MI, MRI, TII);
     }
   }
 
   return Changed;
 }

lib/CodeGen/GlobalISel/LegalizerHelper.cpp

 
 LegalizerHelper::LegalizerHelper(MachineFunction &MF)
     : MRI(MF.getRegInfo()), LI(*MF.getSubtarget().getLegalizerInfo()) {
   MIRBuilder.setMF(MF);
 }
 
 LegalizerHelper::LegalizeResult
 LegalizerHelper::legalizeInstrStep(MachineInstr &MI) {
+  DEBUG(dbgs() << "Legalizing: "; MI.print(dbgs()));
+
   auto Action = LI.getAction(MI, MRI);
   switch (std::get<0>(Action)) {
   case LegalizerInfo::Legal:
+    DEBUG(dbgs() << ".. Already legal\n");
     return AlreadyLegal;
   case LegalizerInfo::Libcall:
+    DEBUG(dbgs() << ".. Convert to libcall\n");
     return libcall(MI);
   case LegalizerInfo::NarrowScalar:
+    DEBUG(dbgs() << ".. Narrow scalar\n");
     return narrowScalar(MI, std::get<1>(Action), std::get<2>(Action));
   case LegalizerInfo::WidenScalar:
+    DEBUG(dbgs() << ".. Widen scalar\n");
     return widenScalar(MI, std::get<1>(Action), std::get<2>(Action));
   case LegalizerInfo::Lower:
+    DEBUG(dbgs() << ".. Lower\n");
     return lower(MI, std::get<1>(Action), std::get<2>(Action));
   case LegalizerInfo::FewerElements:
+    DEBUG(dbgs() << ".. Reduce number of elements\n");
     return fewerElementsVector(MI, std::get<1>(Action), std::get<2>(Action));
   case LegalizerInfo::Custom:
+    DEBUG(dbgs() << ".. Custom legalization\n");
     return LI.legalizeCustom(MI, MRI, MIRBuilder) ? Legalized
                                                   : UnableToLegalize;
   default:
+    DEBUG(dbgs() << ".. Unable to legalize\n");
     return UnableToLegalize;
   }
 }
 
 void LegalizerHelper::extractParts(unsigned Reg, LLT Ty, int NumParts,
                                    SmallVectorImpl<unsigned> &VRegs) {
   for (int i = 0; i < NumParts; ++i)
     VRegs.push_back(MRI.createGenericVirtualRegister(Ty));
   MIRBuilder.buildUnmerge(VRegs, Reg);
 }
 
 static RTLIB::Libcall getRTLibDesc(unsigned Opcode, unsigned Size) {
   switch (Opcode) {