[PATCH] D113116: [CodeGen] Fix assertion failure in TwoAddressInstructionPass::rescheduleMIBelowKill

[Matthias Braun via Phabricator via llvm-commits]

MatzeB added a comment.

Let's fix the condition then this is good to go.



================
Comment at: llvm/lib/CodeGen/TwoAddressInstructionPass.cpp:959
     // We have to move the copies first so that the MBB is still well-formed
     // when calling handleMove().
     for (MachineBasicBlock::iterator MBBI = AfterMI; MBBI != End;) {
----------------
foad wrote:
> RKSimon wrote:
> > Adjust this - since we're not actually moving the debug instr copies 
> Not quite sure what you mean. The instructions in the range [AfterMI,End) are a mixture of copies and debug instructions. We are moving all of them, but (with this patch) only updating LiveIntervals when we move a copy. I think the comment above is still accurate. Did you have a suggestion for improving it?
I also needed a while to understand this comment, since it implies that there are only COPY instructions here. I think you should either tweak the comment to say that there may be debug or pseudo instructions mixed in or add a comment before the `if (isDebugOrPseudoInstr()`.


================
Comment at: llvm/lib/CodeGen/TwoAddressInstructionPass.cpp:963
       MBB->splice(InsertPos, MBB, CopyMI);
-      LIS->handleMove(*CopyMI);
+      if (!CopyMI->isDebugInstr())
+        LIS->handleMove(*CopyMI);
----------------
The condition used in `SlotIndexes.cpp` and in `skipDebugInstructionsForward` (I wish it had a better name) seems to be `isDebugOrPseudoInstr()`.

You could go for `LiveIntervals::hasIndex()` instead to be safe for future changes (at the cost of an extra map lookup). I'll let you make the call depending on how perf critical you judge this part.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113116/new/

https://reviews.llvm.org/D113116