[PATCH] D48331: [DebugInfo][InstCombine] Preserve DI after merging instructions

[Vedant Kumar via Phabricator via llvm-commits]

vsk added subscribers: probinson, mattd, efriedma.
vsk added inline comments.


================
Comment at: lib/Transforms/InstCombine/InstCombineCasts.cpp:280
               Res, DII->getVariable(), DII->getExpression(),
               DII->getDebugLoc().get(), &*std::next(CI.getIterator()));
       }
----------------
There should be a helper function that cuts down on the boilerplate needed to insert fresh debug values.

Here's the usage I envision:
```
InsertReplacementDebugValues(CSrc, Res, &*std::next(CI.getIterator()), [](DbgInfoIntrinsic &OldDII) -> DIExpression * {
  return OldDII.getExpression();
});
```

@aprantl, @mattd and others: wdyt?


================
Comment at: lib/Transforms/InstCombine/InstCombineCasts.cpp:1092
+    // Since the old instruction is merged, we preserve it's DI as
+    // a fragment in the resulting instruction
+    SmallVector<DbgInfoIntrinsic *, 1> SrcDbgInsts;
----------------
Could you make the comment more specific? Example: "Preserve any debug values referring to the zext by ...".


================
Comment at: lib/Transforms/InstCombine/InstCombineCasts.cpp:1099
+        auto Fragment = DIExpression::createFragmentExpression(
+            DII->getExpression(), SrcBitsKept, DestBitSize);
+        DIB.insertDbgValueIntrinsic(
----------------
+ @aprantl and @probinson for dwarf expertise.

There are two cases: a zext can either operate on integers or on vectors of integers. I think the fragment you're creating here is correct in the first case, but not the second. For the second case, I think you'd to emit multiple dbg.values with distinct fragments describing each component of the original vector.


================
Comment at: lib/Transforms/InstCombine/InstCombineCasts.cpp:1099
+        auto Fragment = DIExpression::createFragmentExpression(
+            DII->getExpression(), SrcBitsKept, DestBitSize);
+        DIB.insertDbgValueIntrinsic(
----------------
vsk wrote:
> + @aprantl and @probinson for dwarf expertise.
> 
> There are two cases: a zext can either operate on integers or on vectors of integers. I think the fragment you're creating here is correct in the first case, but not the second. For the second case, I think you'd to emit multiple dbg.values with distinct fragments describing each component of the original vector.
With the helper I suggested, the zext-of-integer case could be simplified to:

```
InsertReplacementDebugValues(Src, Res, &*std::next(CI.getIterator()), [&](DbgInfoIntrinsic &OldDII) -> DIExpression * {
  return DIExpression::createFragmentExpression(OldDII.getExpression(), SrcBitsKept, DestBitSize);
});
```


================
Comment at: lib/Transforms/InstCombine/InstCombineCasts.cpp:1109
+    if (MaskedValueIsZero(
+            Res, APInt::getHighBitsSet(DestBitSize, DestBitSize - SrcBitsKept),
+            0, &CI))
----------------
gramanas wrote:
> This and the following were changed by clang-format, I hope there is no problem formating those, since my work is done nearby.
This does pose a problem. Please avoid re-formatting code unrelated to your patch. This makes diffs harder to read, and makes attribution harder when looking at git history. If you'd like, you can use the clang-format-diff git hook to only reformat that changes you're making.

A simple way to remove these changes from your patch is to upstage your changes, then use `git add -p` to re-stage the chunks you want to keep.


================
Comment at: test/Transforms/InstCombine/debuginfo-trunc-and-zext.ll:1
+; ModuleID = 'debuginfo-trunc-and-zext.ll'
+source_filename = "debuginfo-trunc-and-zext.ll"
----------------
lebedev.ri wrote:
> This does not actually check anything, there is no `RUN` line.
Right. When you add a run line, please reduce this test by invoking `opt -debugify`, instead of checking in unnecessary metadata lines.


Repository:
  rL LLVM

https://reviews.llvm.org/D48331