[GlobalISel] Permit select() to erase.

[ab]

Summary

This lets the selector erase instructions, which is incompatible with reverse iterators. The code is pretty awkward, but I'm not sure there's anything cleaner to be done (short of having select() return iterators, which I'd rather avoid if unnecessary).

This also lets us dump all "selected" instructions (really, just the instructions between prev(MI) and next(MI)); I'll commit separately but these seem worth reviewing together.

WDYT?

[• tstellarAMD]

This patch fixes the crash for me.

[• tstellarAMD]

Ping.

[dberris]

Drive-by review here.

lib/CodeGen/GlobalISel/InstructionSelect.cpp

75–78 ↗

(On Diff #65948)

nit: How about doing the --MII in the for statement?

for (auto MII = ...; !ReachedBegin; --MII) {
  ...
  ReachedBegin = MII == Begin;
  ...
}

[t.p.northover]

I think it's reasonable. I'll commit it later today unless Ahmed beats me to it. Sorry about the delay.

llvm/trunk/lib/CodeGen/GlobalISel/InstructionSelect.cpp

 
 #endif
   // FIXME: We could introduce new blocks and will need to fix the outer loop.
   // Until then, keep track of the number of blocks to assert that we don't.
   const size_t NumBlocks = MF.size();
 
   bool Failed = false;
   for (MachineBasicBlock *MBB : post_order(&MF)) {
-    for (MachineBasicBlock::reverse_iterator MII = MBB->rbegin(),
-                                             End = MBB->rend();
-         MII != End;) {
-      MachineInstr &MI = *MII++;
-      DEBUG(dbgs() << "Selecting: " << MI << '\n');
+    if (MBB->empty())
+      continue;
+
+    // Select instructions in reverse block order. We permit erasing so have
+    // to resort to manually iterating and recognizing the begin (rend) case.
+    bool ReachedBegin = false;
+    for (auto MII = std::prev(MBB->end()), Begin = MBB->begin();
+         !ReachedBegin;) {
+      // Keep track of the insertion range for debug printing.
+      const auto AfterIt = std::next(MII);
+
+      // Select this instruction.
+      MachineInstr &MI = *MII;
+
+      // And have our iterator point to the next instruction, if there is one.
+      if (MII == Begin)
+        ReachedBegin = true;
+      else
+        --MII;
+
+      DEBUG(dbgs() << "Selecting: \n  " << MI);
+
       if (!ISel->select(MI)) {
         if (TPC.isGlobalISelAbortEnabled())
           // FIXME: It would be nice to dump all inserted instructions.  It's
           // not
           // obvious how, esp. considering select() can insert after MI.
           reportSelectionError(MI, "Cannot select");
         Failed = true;
         break;