[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.

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?



================
Comment at: llvm/lib/CodeGen/RemoveRedundantDebugValues.cpp:96-97
+
+      DebugVariable Var(MI.getDebugVariable(), MI.getDebugExpression(),
+                        MI.getDebugLoc()->getInlinedAt());
+      auto VMI = VariableMap.find(Var);
----------------
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.





================
Comment at: llvm/lib/CodeGen/RemoveRedundantDebugValues.cpp:115-117
+    // If this instruction modifies any of variables locations, it
+    // shouldn't be a subject for removing, since a new debug value
+    // is expected in that situtation.
----------------
nit/suggestion: I think this comment could be more concise, maybe something like: "Stop tracking any locations that are clobbered by this instruction."


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105280