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

[Jeremy Morse via Phabricator via llvm-commits]

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

LGTM, thanks for working through this. Do you still need patches to be committed for you?



================
Comment at: llvm/lib/CodeGen/MachineVerifier.cpp:2286-2291
+  MachineBasicBlock::iterator LastPHIIt =
+    std::prev(MBB2.SkipPHIsAndLabels(MBB2.begin()));
+  if (LastPHIIt != skipDebugInstructionsBackward(
+          std::prev(MBB2.SkipPHIsLabelsAndDebug(MBB2.begin())), MBB2.begin()))
+    report("Unexpected DBG_VALUE between PHI and LABEL", &MBB2);
+}
----------------
yechunliang wrote:
> jmorse wrote:
> > Wouldn't a block that has no PHIs or labels end up returning MBB2.begin()? Applying std::prev to that would break.
> > 
> > I get what you're doing here, and it makes sense -- possibly a better way would be to skipDebugIntructionsForward from the position of SkipPHIsAndLabels instead? You don't need to worry about whether the list of instructions is empty that way, because in the worst case everything will end pointing at MBB2.end().
> >Wouldn't a block that has no PHIs or labels end up returning MBB2.begin()? Applying std::prev to that would break.
> there should be at least one PHI on the top of MBB when come into this function. If these is only one PHI instruction, the SkipPHIsAndLabels will return MBB.end(), and std::prev will be MBB.begin(). I though that will not break.
> 
> > I get what you're doing here, and it makes sense -- possibly a better way would be to skipDebugIntructionsForward from the position of SkipPHIsAndLabels instead? You don't need to worry about whether the list of instructions is empty that way, because in the worst case everything will end pointing at MBB2.end().
> 
> Do you mean to use skipDebugInstructionsBackward(SkipPHIsLabelsAndDebug) directly instead of skipDebugInstructionsBackward(std::prev(SkipPHIsLabelsAndDebug))? 
> If so, here is my explanation: the result of these two function is different. In case there is only PHI and Lables in the BasicBlock, then SkipPHIsAndLabels() will return MBB2.end(), and the std::prev will be last PHI or Label, but when call skipDebugInstructionsBackward(MBB2.end()), it will still return MBB2.end(), not the previous LastPHIIt that we expect. The purpose of this code is first to find the lastPHI, then backward to skip debug if the lastPHI is DEB_VALUE.
Ah -- I completely missed the guard at the start of the function, right you are. With the check for empty first, I don't get any test failures at all.


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

https://reviews.llvm.org/D70597