[DebugInfo][InstCombine] Preserve DI after combining zext instructions

[gramanas]

Summary

When zext is EvaluatedInDifferentType, InstCombine
drops the dbg.value intrinsic. This patch tries to
preserve said DI, by inserting the zext's old DI in the
resulting instruction.

The current implementation works only for integer
non-vector variables, but this should get extendet to work for
every case.

[gramanas]

lib/Transforms/InstCombine/InstCombineCasts.cpp

1109 ↗

(On Diff #151962)

This and the following were changed by clang-format, I hope there is no problem formating those, since my work is done nearby.

[lebedev.ri]

test/Transforms/InstCombine/debuginfo-trunc-and-zext.ll

1 ↗

(On Diff #151962)

This does not actually check anything, there is no RUN line.

[gramanas]

Add RUN: clause to the test

[lebedev.ri]

test/Transforms/InstCombine/debuginfo-trunc-and-zext.ll

13–15 ↗

(On Diff #151964)

This only check for the existence of three dbginfo's.
I wonder how much noise would the utils/update_test_checks.py output contain.

[vsk]

lib/Transforms/InstCombine/InstCombineCasts.cpp

280 ↗

(On Diff #151962)

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?

1092 ↗

(On Diff #151962)

Could you make the comment more specific? Example: "Preserve any debug values referring to the zext by ...".

1099 ↗

(On Diff #151962)

+ @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.

1099 ↗

(On Diff #151962)

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);
});

1109 ↗

(On Diff #151962)

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.

test/Transforms/InstCombine/debuginfo-trunc-and-zext.ll

1 ↗

(On Diff #151962)

Right. When you add a run line, please reduce this test by invoking opt -debugify, instead of checking in unnecessary metadata lines.

[gramanas]

Clean up code and test

[probinson]

lib/Transforms/InstCombine/InstCombineCasts.cpp

1092 ↗

(On Diff #152040)

Comment should end with a full-stop. Also it's should be its here.

1099 ↗

(On Diff #151962)

Emitting one fragment per element sounds plausible, but Adrian is really the fragment expert.

[gramanas]

Make use of rL335144 utility.

Add non-vector test.

[gramanas]

Update lamda to return nullptr if it fails to create the fragment.

[gramanas]

Fragments don't seem to be needed here

[gramanas]

• Focus on the scalar test case
• Use fragments since the width of the Src and Res instructions change

[aprantl]

lib/Transforms/InstCombine/InstCombineCasts.cpp

1109 ↗

(On Diff #151962)

If you are going to make a lot of changes to the same file, you can also do a separate commit that is just a clang-format of this file ahead of time.

[vsk]

lib/Transforms/InstCombine/InstCombineCasts.cpp

1093 ↗

(On Diff #152690)

DIExpression doesn't need to be null-checked here because DbgValueInst::getExpression() is always nonnull.

1094 ↗

(On Diff #152690)

You mentioned offline that this causes a verifier failure. If we can't use fragments, consider using a simpler approach for scalars: using an empty expression. It won't work for vectors, but in the scalar case, the high bits should already be cleared. We can relax the check in https://reviews.llvm.org/D48408 to allow this.

1097 ↗

(On Diff #152690)

This unwraps an Optional without a check. If you're sure this can't fail, please add a comment explaining why.

[gramanas]

lib/Transforms/InstCombine/InstCombineCasts.cpp

1094 ↗

(On Diff #152690)

If I use an empty DIExpression it doesn't change the fact that the dbg.value call points to a DILocalVariable that after instcombine has a different DIBasicType than what it used to.

Basiacly the dbg.value call says that the value was changed to an i32 but the it's type is still i8 in the debug info. If there is a way to explain this to the debugger using DIExpressions, I can't find it.

[aprantl]

lib/Transforms/InstCombine/InstCombineCasts.cpp

1094 ↗

(On Diff #152690)

(Not sure if that is your question) We don't have any means to express that the SSA value used by a dbg.value instrinsic is larger (e.g., because of s/zext) than the size of the fragment being described.

For example, if the size of the variable is 8 bits and the value is 32 bits then implicitly the lower 8 bits are used. You could make this explicit by masking out everything but the lower out bits in the DIExpression (DW_OP_constu 0xff DW_OP_and) but this is redundant so we don't do it.

Why would you like to express this?

[gramanas]

lib/Transforms/InstCombine/InstCombineCasts.cpp

1094 ↗

(On Diff #152690)

I thought the fact that a 32 bit variable has DIBasicType of size 8 is wrong. But modifying the DI would interfere with the way the front-end described the program at the first place, thus I was thinking of a way to describe it with a DIExpression. Since the correct value is being displayed implicitly, I guess it's not a problem.

[aprantl]

lib/Transforms/InstCombine/InstCombineCasts.cpp

1094 ↗

(On Diff #152690)

When you say "32 bit variable" are you referring to the source variable or the SSA value that is used by the dbg.value intrinsic?

[gramanas]

lib/Transforms/InstCombine/InstCombineCasts.cpp

1094 ↗

(On Diff #152690)

When running opt -debugify -instcombine to the scalar test below, the zext is replaced with a wider mul (i32 from i8) and the resulting dbg.value from this patch points to a DILocalVariable that has DIBasicType of size 8, while its an i32 value.

I think I mean the SSA value.

[vsk]

lib/Transforms/InstCombine/InstCombineCasts.cpp

1094 ↗

(On Diff #152690)

It looks like it's OK for a integer dbg.value operand to be wider than the type of its associated DILocalVariable. Adrian's second-to-last comment states that the debugger will handle this by displaying the lower bits.

[aprantl]

lib/Transforms/InstCombine/InstCombineCasts.cpp

1094 ↗

(On Diff #152690)

Correct. I should also point out that the following is entirely legal and actually gets emitted by LLVM on i386 for an 8-bit variable in the ah register:

dbg.value(!metadata i32 %value, !metadata "my8bit-variable", !metadata !DIExpression(DW_OP_shr 8)) // = %value >> 8

So we describe an 8-bit variable in bits 8-15 of a. 32-bit value.

[gramanas]

• Update the scalar case according to the comments
• Postpone vector case for a later patch

[vsk]

test/Transforms/InstCombine/debuginfo-trunc-and-zext.ll

17 ↗

(On Diff #153040)

Please move the check line closer to the associated IR. You can pick whether to add the checks after the associated IR lines or before. So:

; CHECK-LABEL: define i32 @test-scalar
define i32 @test-scalar(i32 %x, i32 %y) {

etc.

18 ↗

(On Diff #153040)

This just tests that there are some dbg.values. Please make the test more specific by checking that the operand of the dbg.value is correct.

[bjope]

test/Transforms/InstCombine/debuginfo-trunc-and-zext.ll

1 ↗

(On Diff #151962)

Is that really a good idea?

1. It is really hard to review this without knowing what the input looks like.
2. As this will be a regression test, isn't it better to have the input fixated. Who know what -debugify will generate for this input in the future.
3. There might be lots of redundant debug info generated by -debugify, making it hard to understand the test case if it fails in the future (specially without knowing if -debugify generated something differently originally).

[vsk]

test/Transforms/InstCombine/debuginfo-trunc-and-zext.ll

1 ↗

(On Diff #151962)

The basic requirement of -debugify output is that it must be stable enough to use in regression tests. Just to recap, a dbg.value is inserted after each non-void-valued instruction if the insertion point is valid, and the variable names are sequential integers. Occasionally -debugify requires bug fixes, but these changes aren't allowed to break existing tests or to change their meaning. I hope that addresses your first and second concerns? It does take some extra effort initially to understand this style of test, but I think that effort pays off.

Concretely, the advantage of using -debugify in regression tests is that it lets us test an optimization in the exact same place as the optimization's ability to preserve debug info. This makes it easier to keep debug info tests in sync with the behavior of an optimization. And it makes it much easier to add tests by lessening the need for manual reduction, which is a barrier to contribution.

To your third point, you're right that -debugify inserts more debug info than strictly needed, but we can avoid confusion here by using strict CHECK lines.

[bjope]

test/Transforms/InstCombine/debuginfo-trunc-and-zext.ll

1 ↗

(On Diff #151962)

So the size of the variable always matches the value exactly?
And it will never happen that -debugify will simulate that SROA has splitted an aggregate into several smaller pieces?
And we will never let -debugify generate DIExpressions?

Anyway, my main concern is that it will be really hard to review patches like this one. without knowing what the input looks like. But maybe that is obvious after playing around with debugify for some hours.

[bjope]

test/Transforms/InstCombine/debuginfo-trunc-and-zext.ll

1 ↗

(On Diff #151962)

Concretely, the advantage of using -debugify in regression tests is that it lets us test an optimization in the exact same place as the optimization's ability to preserve debug info.

I guess there is a test case for the optimization somewhere, is the idea that this test shouuld be added as an extra RUN line in that test case instead of adding a new file?

[vsk]

test/Transforms/InstCombine/debuginfo-trunc-and-zext.ll

1 ↗

(On Diff #151962)

So the size of the variable always matches the value exactly?

Yes, specifically, DataLayout::getTypeAllocSizeInBits is used for sized types.

And it will never happen that -debugify will simulate that SROA has splitted an aggregate into several smaller pieces?
And we will never let -debugify generate DIExpressions?

I view these specific changes as a bit out of scope for -debugify, but regardless, it's possible to write regression tests which don't depend on these details.

Anyway, my main concern is that it will be really hard to review patches like this one. without knowing what the input looks like. But maybe that is obvious after playing around with debugify for some hours.

I'm.. growing increasingly concerned about this as well. I'd like our tests to be reasonably simple to understand. My view is that these tests already require understanding of at least one pass (in this case -instcombine), so it's reasonable to depend on a stable testing pass on top of that. That could well be too optimistic. How much extra effort is involved in parsing this test? Would more documentation help? (Anyone else have thoughts about this?)

Concretely, the advantage of using -debugify in regression tests is that it lets us test an optimization in the exact same place as the optimization's ability to preserve debug info.

I guess there is a test case for the optimization somewhere, is the idea that this test shouuld be added as an extra RUN line in that test case instead of adding a new file?

Ideally yes. If you grep in tests/Transforms you'll see this pattern used to test gvn, *dce, sccp, licm etc. I would like this patch to adopt the same approach, similar to https://reviews.llvm.org/D48640.

[bjope]

test/Transforms/InstCombine/debuginfo-trunc-and-zext.ll

1 ↗

(On Diff #151962)

Ok, I think I got it. Thanks to @vsk for the explanations! And sorry @gramanas for having this discussion as part of reviewing your patch.

[gramanas]

test/Transforms/InstCombine/debuginfo-trunc-and-zext.ll

1 ↗

(On Diff #151962)

It's not a problem at all. Thanks for your comments, any feedback is welcomed.

I would like this patch to adopt the same approach, similar to https://reviews.llvm.org/D48640.

Indeed the test should get incorporated with already existing tests. I will work on that.

Anyway, my main concern is that it will be really hard to review patches like this one. without knowing what the input looks like. But maybe that is obvious after playing around with debugify for some hours.

I'm.. growing increasingly concerned about this as well. I'd like our tests to be reasonably simple to understand. My view is that these tests already require understanding of at least one pass (in this case -instcombine), so it's reasonable to depend on a stable testing pass on top of that. That could well be too optimistic. How much extra effort is involved in parsing this test? Would more documentation help? (Anyone else have thoughts about this?)

I think the -debugify pass is pretty simple to understand, but some documentation would definitely help future devs who come across this kind of tests, or would like to add similar tests to other passes. Since getting the -debugify output is just an opt invocation away, I think it's better to write tests like this instead of incorporating the debug info in the .ll file which results in clutter.

[gramanas]

Incorporate the -debugify test into the existing test files.

[gramanas]

test/Transforms/InstCombine/cast-mul-select.ll

3 ↗

(On Diff #153303)

Should I add a note saying that the DBGINFO checks are not autogenerated?

[bjope]

In D48331#1146563, @gramanas wrote:

Incorporate the -debugify test into the existing test files.

Nice! I was a little bit confused the other day, but right now I got a very good feeling about this.

[vsk]

lib/Transforms/InstCombine/InstCombineCasts.cpp

1084 ↗

(On Diff #153303)

Nit -- please capitalize this and use the same line wrapping as the surrounding code.

test/Transforms/InstCombine/cast-mul-select.ll

3 ↗

(On Diff #153303)

No, but please remove the note about the checks being autogenerated, because with this change they no longer are.

20 ↗

(On Diff #153303)

We don't gain much by capturing DILocVar{C,D} here. It's fine to not do this, because we don't need to check what's inside the variables.

[gramanas]

Addressing inline comments.

[vsk]

LGTM, thanks! We can simplify this a bit with D48676 but it doesn't need to be a blocker.

llvm/trunk/lib/Transforms/InstCombine/InstCombineCasts.cpp

if ((DestTy->isVectorTy() || shouldChangeType(SrcTy, DestTy)) &&
     // Okay, we can transform this!  Insert the new expression now.
     LLVM_DEBUG(
         dbgs() << "ICE: EvaluateInDifferentType converting expression type"
                   " to avoid zero extend: "
                << CI << '\n');
     Value *Res = EvaluateInDifferentType(Src, DestTy, false);
     assert(Res->getType() == DestTy);
 
+    // When DestTy is integer, try to preserve any debug values referring
+    // to the zext being replaced.
+    // TODO: This should work for vectors as well, possibly via the use
+    // of DWARF fragments.
+    if (DestTy->isIntegerTy()) {
+      insertReplacementDbgValues(
+          *Src, *Res, CI, [](DbgInfoIntrinsic &OldDII) -> DIExpression * {
+            return OldDII.getExpression();
+          });
+    }
+
     uint32_t SrcBitsKept = SrcTy->getScalarSizeInBits()-BitsToClear;
     uint32_t DestBitSize = DestTy->getScalarSizeInBits();
 
     // If the high bits are already filled with zeros, just replace this
     // cast with the result.
     if (MaskedValueIsZero(Res,
                           APInt::getHighBitsSet(DestBitSize,
                                                 DestBitSize-SrcBitsKept),

llvm/trunk/test/Transforms/InstCombine/cast-mul-select.ll

-; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt < %s -instcombine -S | FileCheck %s
+; RUN: opt -debugify -instcombine -S < %s | FileCheck %s -check-prefix DBGINFO
 
 target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32"
 
 define i32 @mul(i32 %x, i32 %y) {
 ; CHECK-LABEL: @mul(
 ; CHECK-NEXT:    [[C:%.*]] = mul i32 [[X:%.*]], [[Y:%.*]]
 ; CHECK-NEXT:    [[D:%.*]] = and i32 [[C]], 255
 ; CHECK-NEXT:    ret i32 [[D]]
-;
+
+; Test that when zext is evaluated in different type
+; we preserve the debug information in the resulting
+; instruction.
+; DBGINFO-LABEL: @mul(
+; DBGINFO-NEXT:    [[C:%.*]] = mul i32 {{.*}}
+; DBGINFO-NEXT:    call void @llvm.dbg.value(metadata i32 [[C]]
+; DBGINFO-NEXT:    [[D:%.*]] = and i32 {{.*}}
+; DBGINFO-NEXT:    call void @llvm.dbg.value(metadata i32 [[D]]
+
   %A = trunc i32 %x to i8
   %B = trunc i32 %y to i8
   %C = mul i8 %A, %B
   %D = zext i8 %C to i32
   ret i32 %D
 }
 
 define i32 @select1(i1 %cond, i32 %x, i32 %y, i32 %z) {