[PATCH] D48154: [VirtRegRewriter] Avoid clobbering registers when expanding copy bundles

[Matthias Braun via Phabricator via llvm-commits]

MatzeB accepted this revision.
MatzeB added a comment.
This revision is now accepted and ready to land.

LGTM with this round of nitpicks addressed.

The re-scheduling code is quite tricky so I hope I didn't miss something. On the other hand I don't have an idea on how to make it simpler (except the small stuff mentioned).



================
Comment at: lib/CodeGen/VirtRegMap.cpp:426
+      for (const MachineInstr *Src : Srcs)
+        if (Src != Dst)
+          if (TRI->regsOverlap(Dst->getOperand(0).getReg(),
----------------
I think regsOverlap also has a Src==Dst shortcut, so this should be unnecessary. So the 2 ifs can shrink to `return TRI->regsOverlap()`


================
Comment at: lib/CodeGen/VirtRegMap.cpp:451
+    MachineInstr *BundleStart = FirstMI;
+    for (MachineInstr *Inst : llvm::reverse(MIs)) {
+      // If instruction is in the middle of the bundle, move it before the
----------------
Maybe give this a more specific name like `BundledMI`?


================
Comment at: lib/CodeGen/VirtRegMap.cpp:455-461
+      if (Inst != BundleStart) {
+        Inst->removeFromBundle();
+        MBB.insert(FirstMI->getIterator(), Inst);
+      } else if (Inst->isBundledWithSucc()) {
+        Inst->unbundleFromSucc();
+        BundleStart = &*std::next(Inst->getIterator());
+      }
----------------
I guess this could just be `Inst->removeFromBundle()` in both cases (to simplify the code).


================
Comment at: lib/CodeGen/VirtRegMap.cpp:457
+        Inst->removeFromBundle();
+        MBB.insert(FirstMI->getIterator(), Inst);
+      } else if (Inst->isBundledWithSucc()) {
----------------
Could you try if this works without `->getIterator()` I think it implicitely converts at least in some situations.


https://reviews.llvm.org/D48154