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

[Orlando Cazalet-Hyams via Phabricator via llvm-commits]

Orlando added a comment.

Thank you for sharing the stats, they look great (both for NFC-ness and with entry-values)! There is an absolutely miniscule shift in numbers in the middle buckets in the non-entry-value case (NFC), but this is to be expected and acceptable imo.

LGTM - Same as for D105279 <https://reviews.llvm.org/D105279> I'll give others a chance to comment before hitting Accept.



================
Comment at: llvm/lib/CodeGen/RemoveRedundantDebugValues.cpp:96-97
+
+      DebugVariable Var(MI.getDebugVariable(), MI.getDebugExpression(),
+                        MI.getDebugLoc()->getInlinedAt());
+      auto VMI = VariableMap.find(Var);
----------------
djtodoro wrote:
> 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!
Please can you add a test for this too? Sorry - I should've asked for this in my first comment.




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

https://reviews.llvm.org/D105280