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

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added a comment.

Looking good, this is almost there (see inline),



================
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) {
----------------
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).


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


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

https://reviews.llvm.org/D70597