[PATCH] D38359: [X86] Ignore DBG instructions in X86CmovConversion optimization to resolve PR34565

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

Is there any way that the debug instructions can become invalid due to this transform? I'm not seeing anything obvious, but wanted to make sure you've thought about this too.



================
Comment at: lib/Target/X86/X86CmovConversion.cpp:621-630
+  // If there are DBG instructions between first CMOV and last CMOV, move them
+  // to after the last CMOV.
+  MachineBasicBlock::iterator DbgIt = MI.getIterator();
+  MachineBasicBlock::iterator DbgE = LastCMOV->getIterator();
+  while (DbgIt != DbgE) {
+    MachineInstr &I = *DbgIt++;
+    if (I.isDebugValue()) {
----------------
I think the use of `MI` to refer to the front of the group makes this and other code in this function harder to read.

I'd suggest renaming that variable to something less confusing (or completely removing it).

But you also have both `It` and `DbgIt` here which also seems confusing.

Really, all of this is driving me toward the conclusion that this is too late to do this fix. Consider that this is after the scan to swap the CC above which seems like it would not naively expect a non-cmov machine intsr. I could well imagine an assert being added to it later that would fire.

I think you want to do this before you start to reason about the group of machine instructions as definitely consecutive `cmov` instructions. I'd also suggest pulling it into its own function.

Then you can write the code in a more clear way along the lines of:

  for (auto I = Group.front().getIterator(), E = Group.back().getIterator(); I != E;) {
    MachineInstr &MI = *I++;
    if (!I.isDebugValue())
      continue;

    // Splice the debug instruction after the cmov group.
    MBB->insertAfter(E, I.removeFromParent());
  }

Anyways, my goal would be to use `auto` and for loop syntax to make the code a bit more clear, but I don't think it'll work well until this gets hoisted out of this location and into another location. Maybe you can make it work over an MI range as well, unsure.



https://reviews.llvm.org/D38359