[PATCH] D52887: [DebugInfo][Dexter] Unreachable line stepped onto after SimplifyCFG.

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added a comment.

The code looks fine, I have some comments about the comments.



================
Comment at: lib/Transforms/Utils/Local.cpp:2535
+  // Since we are moving the instructions out of its basic block, we do not
+  // retain their original debug location and debug intrinsic instructions.
+  // Doing so would degrade the debugging experience and adversely affect the
----------------
Please also explain why if possible. I think the reason is different for DILoctions and dbg.values.


================
Comment at: lib/Transforms/Utils/Local.cpp:2538
+  // accuracy of profiling information.
+  // There are some discussions (PR39141) about extending llvm.dbg.value to
+  // take more than one LLVM SSA value instead of dropping debug info.
----------------
```TODO: Extend llvm.dbg.value to take more than one SSA Value (PR39141) to encode predicated DIExpressions that yield different results on different code paths.```


================
Comment at: lib/Transforms/Utils/Local.cpp:2540
+  // take more than one LLVM SSA value instead of dropping debug info.
+  // For now, when Use the insertion point debug location.
+  for (auto &I : *IfBlock) {
----------------
this sentence is garbled :-)

If I parsed the sentence correctly it sounds like a non-sequitur to me: having conditional dbg.values won't affect our options for DILocations at all. Or are you using "location" in the .debug_loc sense? That's confusing, let's use dbg.value instead.


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:2373
+  if (IfBlock1)
+    promotePHINodes(DomBlock,InsertPt,IfBlock1);
+  if (IfBlock2)
----------------
please clang-format.


================
Comment at: test/CodeGen/X86/pr38762.ll:108
+!34 = !DILocation(line: 12, column: 1, scope: !7)
+!35 = !DILocation(line: 11, column: 3, scope: !7)
----------------
Perhaps compile with `-gno-column-info`, so the test input isn't quite as long?


Repository:
  rL LLVM

https://reviews.llvm.org/D52887