[PATCH] D70597: [PHIEliminate] skip dbg instruction when LowerPHINode

[Chris Ye via Phabricator via llvm-commits]

yechunliang added a comment.

Just updated the code [Diff 233770] follow the comments by jmose and bjope.
I try to add below reference code to MachineVerifier::checkPHIOps, but meet a build error. In the function the type of MBB.begin() is const_iterator, but type of SkipPHIsAndLabels argument is (iterator I). How to convert const_iterator to iterator and pass to SkipPHIsAndLabels? Could I write the MachineVerifier as below code?

  diff --git a/llvm/lib/CodeGen/MachineVerifier.cpp b/llvm/lib/CodeGen/MachineVerifier.cpp
  index ca57e51268e..fbec123a2ec 100644
  --- a/llvm/lib/CodeGen/MachineVerifier.cpp
  +++ b/llvm/lib/CodeGen/MachineVerifier.cpp
  @@ -2273,6 +2273,13 @@ void MachineVerifier::checkPHIOps(const MachineBasicBlock &MBB) {
         }
       }
     }
  +
  +  // Check there must be no dbg_values between PHI and labels
  +  MachineBasicBlock::iterator LastPHIIt =
  +    std::prev(MBB.SkipPHIsAndLabels(MBB.begin()));
  +  if (LastPHIIt == skipDebugInstructionsBackward(
  +          std::prev(MBB.SkipPHIsLabelsAndDebug(MBB.begin())), MBB.begin()))
  +    report("Unexpected dbg_values between PHI and labels", &MBB);
   }



================
Comment at: llvm/lib/CodeGen/PHIElimination.cpp:210-220
+  // LastPHIIt - Fix PR16508:
+  //   When phis get lowered, destination copies are inserted using an iterator that is
+  //   determined once for all phis in the block, which BuildMI interprets as a request
+  //   to insert an instruction directly before the iterator. In the case of a cyclic
+  //   phi, source copies may also be inserted directly before this iterator, which can
+  //   cause source copies to be inserted before destination copies. The fix is to keep
+  //   an iterator to the last phi and then advance it while lowering each phi in order
----------------
jmorse wrote:
> As we've designed the original problem out, and the PHI Elimination pass isn't actually changed now, isn't this comment un-necessary?
> 
> If I understand correctly, what we could say here is something like "There should be no PHIs or Labels at all after this point".
The comments is searched from old commits, and described why need to use LastPHIIt(prev-next), and why need to lower PHI after Label.
This is not related with new patch, I'll remove it. 


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

https://reviews.llvm.org/D70597