[PATCH] D42566: [InstCombine] Preserve debug values for eliminable casts

[Vedant Kumar via Phabricator via llvm-commits]

vsk added a comment.

In https://reviews.llvm.org/D42566#989110, @aprantl wrote:

> Could you achieve a reasonably similar effect with less code by invoking salvageDebugInfo() when the cast is actually eliminated?


I don't think so, because `salvageDebugInfo(DyingInst)` isn't aware of the instruction that replaces the dying instruction. We could introduce a new utility, e.g `forwardDebugInfo(DyingInst, ReplacementInst)`, but I'm not sure whether it would be sufficiently general.



================
Comment at: lib/Transforms/InstCombine/InstCombineCasts.cpp:275
+      findDbgUsers(CSrcDbgInsts, CSrc);
+      if (CSrcDbgInsts.size()) {
+        DIBuilder DIB(*CI.getModule());
----------------
JDevlieghere wrote:
> Out of curiosity: why check `size()` and not `empty()`? Although irrelevant, the STL only guarantees that `empty()` is `O(1)`, but it's why it caught my eye here. :-) 
No particular reason, it's O(1) so it should be OK.


================
Comment at: lib/Transforms/InstCombine/InstCombineCasts.cpp:280
+              Res, DII->getVariable(), DII->getExpression(),
+              DII->getDebugLoc().get(), &*std::next(CI.getIterator()));
+      }
----------------
aprantl wrote:
> The insertion point should be directly before or after DII. The position of the dbg.value intrinsic is significant as it determines when the variable will be visible in the debugger.
I don't think the insertion point can be directly before or after DII.

Take a look at the change to the "alloca-cast-debuginfo.ll" test below: inserting a dbg.value directly before/after DII would introduce a use of [[simplified]] before it's def.

The current code puts the fresh dbg.value in the exact spot where the dying cast is. Doesn't that reflect when the variable is visible precisely?


https://reviews.llvm.org/D42566