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

[Chris Ye via Phabricator via llvm-commits]

yechunliang marked 2 inline comments as done.
yechunliang added a comment.

In D70597#1807759 <https://reviews.llvm.org/D70597#1807759>, @jmorse wrote:

> Running check-llvm, there are a bunch of crashes, for example llvm/test/CodeGen/X86/win-catchpad.ll so there's certainly something breaking there.


The root cause is that the empty MBB have not been considered, so that the MBB.begin() run fails.
I'll update the code and run check-llvm next time before submit patch, thanks.



================
Comment at: llvm/lib/CodeGen/MachineVerifier.cpp:2279-2280
 
+// Check there must be no dbg_values between PHI and labels,
+// so the PHIs could lower after LABELs successfully without dbg barriers.
+void MachineVerifier::checkPHIDbg(const MachineBasicBlock &MBB) {
----------------
jmorse wrote:
> Good concise comment -- just a nit, could you use DBG_VALUEs instead of dbg_values? (It helps people searching for debug related stuff to keep it consistent).
thanks for comment, I'll fix it.


================
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);
+}
----------------
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.


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

https://reviews.llvm.org/D70597