[PATCH] D44610: [SelectionDAG] Support multiple dangling debug info for one value

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h:120
+  /// DanglingDebugInfoVector - Helper type for DanglingDebugInfoMap.
+  typedef SmallVector<DanglingDebugInfo, 4> DanglingDebugInfoVector;
+
----------------
rnk wrote:
> bjope wrote:
> > rnk wrote:
> > > Embedding SmallVector in DenseMap is usually an efficiency trap. DenseMap stores keys and values in the table, not in separate allocations, so the value of every unfilled hashtable entry is wasted. Even then, filled slots typically have one dbg.value.
> > > 
> > > TinyPtrVector was created for this use case, but it looks like the elements are not pointers so it will not work.
> > > 
> > > I guess I'd suggest using std::vector or `SmallVector<DangingDebugInfo, 0>` to minimize wasted inline storage.
> > Thanks a lot for the feedback. I was a little bit worried about this part. I haven't spent any time on optimizing it really, and this first draft was aiming at getting it working in some simple way.
> > 
> > Now when I got the feedback that this patch is on track regarding the functionality I will go on doing some tests regarding impact on compile speed/memory consumption. After all, these problems were seen when analysing https://bugs.llvm.org/show_bug.cgi?id=36417, so it would be a pity if we end up with bad compilation performance again when solving this problem.
> That's always possible, but I suspect any perf degradation will come from the extra DBG_VALUE MachineInstrs inserted after this pass, and not this data structure. Anyway, SmallVector 0 is three pointers big, and DanglingDbgInfo is three, so in the common case, there's not much change.
> 
> One way to build confidence that this isn't introducing a major regression is to measure the time to compile the optimized sqlite3.c amalgamation with debug info before and after your change. You can use `perf -r 5` on Linux to get error bars to get a sense of how precise your comparison is, and whether it's in the noise or not.
std:vector seems to use less heap (compared to SmallVector) when running the small test/DebugInfo/X86/sdag-dangling-dbgvalue.ll test case.

The sqlite3.c test does not seem to result in any dangling debug info during the initial SDAG build, so this patch has no impact really.

When considering C++ and this pointers, as mentioned earlier by someone, we do not get any dangling debug info afaict. The `this` pointer will be an input arguement to the function, so the value is selected before any dbg.value instructions that reference it.

Normally the amount of dangling debug info should be quite small (there could be some nasty code out there so you never now for the general case). I do not think memory consumption should be a problem. The costly part (CPU wise) is probably the linear search in SelectionDAGBuilder::dropDanglingDebugInfo to find all dangling debug info that needs to be dropped. Now we can have more dangling debug info nodes to inspect, although the complexity is the same.

I'll will replace the the SmallVector by a std::vector and then I assume that I can land this.


Repository:
  rL LLVM

https://reviews.llvm.org/D44610