[PATCH] [DebugInfo][FastISel] Prevent using debug location from previous block for local values

[Frédéric Riss]

> On May 20, 2015, at 10:38 AM, Sergey Dmitrouk <sdmitrouk at accesssoftek.com> wrote:
> 
> Hi mcrosier, probinson, dblaikie, echristo,
> 
> As FastISel groups local values at the top of each block and doesn't attach any
> debug location to them (it's intensionally cleared to prevent jumping back and
> forth in debuggers), the following situation is possible:
> 
> ```
> BB#0:
>    .loc 1 2 3
>    ...
> 
> BB#1:
>    <constant load>
>    .loc 1 3 4
>    ...
> ```
> 
> Here `<constant load>` "accidentally" gets wrong debug location from the
> preceding block, which is wrong.

I played with that some time ago and it’s still sitting somewhere on my things-
to-look-at list. Some notes about that bellow.

> This patch adds basic block post-processing for `FastISel`, which will look for
> first instruction with non-null debug location and assign the location to first
> local value (if one exists), producing the following:

Some heuristic has already been proposed for this, see:
https://llvm.org/bugs/show_bug.cgi?id=14501 <https://llvm.org/bugs/show_bug.cgi?id=14501>
I think the patch it refers to is:
http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20120917/151433.html <http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20120917/151433.html>

I think the heuristic was basically the same and echristo rejected it at the time, but
maybe he changed his mind?

Coming back to the problem at hand, it is indeed pretty hard to solve at the
LocalValueMap level. Just playing with it at the time, I tried to emit the constants
at the insertion point rather than in the LocalValue area at the top at the block and
then hoist the instructions if the constant gets reused (updating the debug info). This
doesn’t work because the materialization of a constant might recursively trigger the
materialization of another one, creating a dependency between the constants. If we
ever wanted to modify the logic to emit the constants near there first use, they’d need
to be handled in groups of dependent values.

Another (simpler) approach that I started to investigate, was to remove the LocalValueMap
altogether. I started gathering some numbers to back that route, but I got diverted at that
point and never came back to it. On a full clang build in debug mode, the searches for
constants in the LocalValueMap had an overall  hit rate of approximately 5%.
This number alone doesn’t mean much, it would need to be compared to the relative cost
of creating/emitting the instructions. It’s also only representative of one use case on one
architecture (Other architectures would need to be benchmarked, as well as JITs like the FTL
use case on x86_64). I still think this could be a viable option, but it requires a lot of
benchmarking to convince people.

One thing that isn’t immediately apparent too, is that this bug can lead to some pretty serious
debug experience issues. If you debug code using libcxx in lldb (don’t know about gdb), you
might sometimes run into a mis-behaving ‘next’ over a STL function that would fail to next
over the function and stop somewhere inside it. It tracked some of these back to this
behavior. Having a block implicitly ‘extend’ its range till the end of the local value area of the
next block creates a discrepancy between the scope structure described by the DIEs
(AT_ranges attributes) and the line table. lldb seems to get confused when one line start in
one block but ends in another. This happens even at O0, because libcxx has a lot of
‘always_inline’ attributes.

Fred

> ```
> BB#0:
>    .loc 1 2 3
>    ...
> 
> BB#1:
>    .loc 1 3 4
>    <constant load>
>    .loc 1 3 4
>    ...
> ```
> 
> REPOSITORY
>  rL LLVM
> 
> http://reviews.llvm.org/D9887
> 
> Files:
>  include/llvm/CodeGen/FastISel.h
>  lib/CodeGen/SelectionDAG/FastISel.cpp
>  lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
>  test/DebugInfo/ARM/single-constant-use-preserves-dbgloc.ll
> 
> Index: include/llvm/CodeGen/FastISel.h
> ===================================================================
> --- include/llvm/CodeGen/FastISel.h
> +++ include/llvm/CodeGen/FastISel.h
> @@ -226,6 +226,9 @@
>   /// be appended, and clear the local CSE map.
>   void startNewBlock();
> 
> +  /// \brief Performs post-processing of complete basic block.
> +  void finishNewBlock();
> +
>   /// \brief Return current debug location information.
>   DebugLoc getCurDebugLoc() const { return DbgLoc; }
> 
> @@ -569,6 +572,10 @@
>   bool lowerCallOperands(const CallInst *CI, unsigned ArgIdx, unsigned NumArgs,
>                          const Value *Callee, bool ForceRetVoidTy,
>                          CallLoweringInfo &CLI);
> +
> +  /// \brief Copies first non-empty debug location from list of instructions as
> +  /// starting location for current block.
> +  void fixupLocalValueLocations();
> };
> 
> } // end namespace llvm
> Index: lib/CodeGen/SelectionDAG/FastISel.cpp
> ===================================================================
> --- lib/CodeGen/SelectionDAG/FastISel.cpp
> +++ lib/CodeGen/SelectionDAG/FastISel.cpp
> @@ -104,6 +104,35 @@
>   LastLocalValue = EmitStartPt;
> }
> 
> +void FastISel::finishNewBlock() {
> +  fixupLocalValueLocations();
> +}
> +
> +void FastISel::fixupLocalValueLocations() {
> +  // Nothing to do if we didn't emit any local values.
> +  if (LocalValueMap.empty())
> +    return;
> +
> +  // Skip entry basic block to do not move "prologue_end" location above stack
> +  // adjustment.
> +  if (FuncInfo.MBB == FuncInfo.MBBMap[&FuncInfo.Fn->getEntryBlock()])
> +    return;
> +
> +  // Find first local value by skipping past any EH_LABELs, which go before
> +  // local values.
> +  MachineBasicBlock::iterator FirstLocalValue = FuncInfo.MBB->begin();
> +  while (FirstLocalValue->getOpcode() == TargetOpcode::EH_LABEL)
> +    ++FirstLocalValue;
> +
> +  // Find first instruction with non-null debug location.
> +  MachineBasicBlock::iterator InstWithLoc = FuncInfo.InsertPt;
> +  while (InstWithLoc != FuncInfo.MBB->end() && !InstWithLoc->getDebugLoc())
> +    ++InstWithLoc;
> +
> +  if (InstWithLoc != FuncInfo.MBB->end())
> +    FirstLocalValue->setDebugLoc(InstWithLoc->getDebugLoc());
> +}
> +
> bool FastISel::lowerArguments() {
>   if (!FuncInfo.CanLowerReturn)
>     // Fallback to SDISel argument lowering code to deal with sret pointer
> Index: lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
> ===================================================================
> --- lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
> +++ lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
> @@ -1312,6 +1312,8 @@
>     }
> 
>     FinishBasicBlock();
> +    if (FastIS)
> +      FastIS->finishNewBlock();
>     FuncInfo->PHINodesToUpdate.clear();
>   }
> 
> Index: test/DebugInfo/ARM/single-constant-use-preserves-dbgloc.ll
> ===================================================================
> --- test/DebugInfo/ARM/single-constant-use-preserves-dbgloc.ll
> +++ test/DebugInfo/ARM/single-constant-use-preserves-dbgloc.ll
> @@ -1,4 +1,5 @@
> ; RUN: llc -filetype=asm -asm-verbose=0 < %s | FileCheck %s
> +; RUN: llc -fast-isel -filetype=asm -asm-verbose=0 < %s | FileCheck %s
> 
> ; int main()
> ; {
> 
> EMAIL PREFERENCES
>  http://reviews.llvm.org/settings/panel/emailpreferences/
> <D9887.26161.patch>_______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150520/4568101a/attachment.html>