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

[Carlos Alberto Enciso via Phabricator via llvm-commits]

CarlosAlbertoEnciso marked 7 inline comments as done.
CarlosAlbertoEnciso added inline comments.


================
Comment at: include/llvm/Transforms/Utils/Local.h:454
+/// their debug intrinsic instructions are removed.
+void promotePHINodes(BasicBlock *DomBlock,Instruction *InsertPt,
+                     BasicBlock *IfBlock);
----------------
vsk wrote:
> I think function name and comments here could be clearer.
> 
> The action seems closer to 'hoisting' than 'promoting'. And the function is hoisting all of the instructions in the IfBlock, not just the phis. How about: 'hoistAllInstructionsInto'?
Taking your suggested function name `hoistAllInstructionsInto` . Is a better description for what the function does. I have made some changes to the comments, hoping for more clarity.


================
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
----------------
vsk wrote:
> aprantl wrote:
> > Please also explain why if possible. I think the reason is different for DILoctions and dbg.values.
> Right. Moving locations around degrades single-stepping and profiling. Moving dbg.values around makes variable updates appear re-ordered in a debug session.
Updated the comments to include a better explanation about the actions taken on DILocations and dbg.values.


================
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
----------------
CarlosAlbertoEnciso wrote:
> vsk wrote:
> > aprantl wrote:
> > > Please also explain why if possible. I think the reason is different for DILoctions and dbg.values.
> > Right. Moving locations around degrades single-stepping and profiling. Moving dbg.values around makes variable updates appear re-ordered in a debug session.
> Updated the comments to include a better explanation about the actions taken on DILocations and dbg.values.
Taking your feedback, as the base for the updated comments.


================
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.
----------------
aprantl wrote:
> ```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.```
Included your description about the TODO work.


================
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) {
----------------
aprantl wrote:
> 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.
> this sentence is garbled :-)

Now reading my description, I realized that something was missing and it does not make much sense :-)

I have updated the comments and made a clear distinction between debug locations (DILocations) and debug.values.





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


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


Repository:
  rL LLVM

https://reviews.llvm.org/D52887