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

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added a comment.

Great to hear that this is fixing the issue. I agree with Björn, there should be a MachineVerifier check too that DBG_VALUEs are not mixed with labels. That'll make it very easy to detect this issue reoccurring.



================
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
----------------
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".


================
Comment at: llvm/lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp:944
+    //  LABEL
+    //  DBG_VALUE
+    MachineBasicBlock::iterator BBBegin = BB->SkipPHIsAndLabels(BB->begin());
----------------
I think this comment could be shorter, how about, "Always place DBG_VALUEs after all other special start-of-block instructions, to avoid debug data hiding start-of-block instructions."


================
Comment at: llvm/test/CodeGen/X86/dbg-changes-codegen-phi-elimination.ll:21
+
+define i32 @func(i32 %a0) personality void ()* @personality {
+bb1:
----------------
nit: this should have !dbg !9 after it, right? To associate the function with the DISubprogram?


================
Comment at: llvm/test/CodeGen/X86/dbg-changes-codegen-phi-elimination.ll:54-55
+
+attributes #0 = { nounwind readnone speculatable willreturn }
+attributes #1 = { nounwind }
+
----------------
If these attributes aren't important to the test, they should be removed.


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

https://reviews.llvm.org/D70597