[PATCH] D105280: [2/2][RemoveRedundantDebugValues] Introduce a MIR pass that removes redundant DBG_VALUEs

[Djordje Todorovic via Phabricator via llvm-commits]

djtodoro added a comment.

In D105280#2853116 <https://reviews.llvm.org/D105280#2853116>, @Orlando wrote:

> Hi @djtodoro, thank you for implementing this! I've left some inline comments.
>
> I wonder if it would it be worth getting an `llvm-locstats` comparison on a codebase built with and without this patch-set to get confidence that it is a NFC?

Thanks for your comments!
Actually, the `llvm-locstats` numbers will improve, since this fixes MIR, and `LiveDebugValues` will generate more entry-values for example, since the analysis generates entry-values for unmodified parameters, but with these duplicates, we have had a lot of false positives with the respect to param's changing (the pass considered a new/redundant `DBG_VALUE` as an assignment). I'll share the locstats numbers anyway.



================
Comment at: llvm/lib/CodeGen/RemoveRedundantDebugValues.cpp:96-97
+
+      DebugVariable Var(MI.getDebugVariable(), MI.getDebugExpression(),
+                        MI.getDebugLoc()->getInlinedAt());
+      auto VMI = VariableMap.find(Var);
----------------
Orlando wrote:
> I think you need to pass `None` as the second argument to the `DebugVariable` ctor (i.e. using the overload that takes an `Optional<FragmentInfo>`).
> 
> Otherwise this function may remove a DBG_VALUE for a variable fragment that overlaps with the fragment of a preceding DBG_VALUE because their FragmentInfo doesn't match. This would result in incorrect debugging information.
> 
> In the example below, `reduceDbgValsForwardScan` will currently (wrongly) remove DBG_VALUE 3 because 1 and 3 have the same variable location, and DBG_VALUE 2 has a different fragment so it is not found in the VariableMap lookup. This causes an inaccuracy, because DBG_VALUE 2 implicitly describes a new variable location for the fragment {0, 32} which means that DBG_VALUE 3 is not actually redundant.
> ```
> 1  DBG_VALUE $rax, "x", DIExpression(DW_OP_fragment, 0, 32)
>    ...
> 2  DBG_VALUE $rdi, "x", DIExpression()
>    ...
> 3  DBG_VALUE $rax, "x", DIExpression(DW_OP_fragment, 0, 32)
> ```
> 
> Using `None` for the `Optional<FragmentInfo>` means that the lookup will succeed for a {variable, inlined-at} pair regardless of the fragment. The downside is that this approach is slightly over-conservative; a DBG_VALUE may end up "blocking" the valid removal of a subsequent DBG_VALUE for the same variable with a non-overlapping fragment. But this is better than introducing inaccurate debug info IMO. Making this change would then match how 
>  `removeRedundantDbgInstrsUsingForwardScan` in D71478 is implemented. Note that, as in D71478, you only need to do this in the forward scan.
> 
> 
> 
Nice catch! Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105280