[Util] Refer to [s|z]exts of args when converting dbg.declares (fix PR35400)

[vsk]

Summary

When converting dbg.declares, if the described value is a [s|z]ext,
refer to the ext directly instead of referring to its operand.

This fixes a narrowing bug (the debugger got the sign of a variable
wrong, see llvm.org/PR35400).

The main reason to refer to the ext's operand was that an optimization
may remove the ext itself, leading to a dropped variable. Now that
InstCombine has been taught to use replaceAllDbgUsesWith (r336451), this
is less of a concern. Other passes can/should adopt this API as needed
to fix dropped variable bugs.

[aprantl]

I like the amount of red in this patch :-)

llvm/test/Transforms/Util/dbg-user-of-aext.ll

43

This is actually changing codegen. Is that intended?

[vsk]

llvm/test/Transforms/Util/dbg-user-of-aext.ll

43

Yes, for two reasons:

1. The sext path wasn't covered before (afaict), and
2. To highlight that the prior behavior is actually incorrect specifically for sexts (see pr35400).

[vsk]

Ping.

[vsk]

llvm/test/Transforms/Util/dbg-user-of-aext.ll

43

I think my response here may have been confusing. Just to be clear: this patch doesn't change codegen in the traditional sense (apart from changing dbg.values). The only reason I changed the zext to a sext here was to get slightly better code coverage.

[vsk]

Apologies, I haven't been able to get back to this. I'd be happy to have someone commandeer this.

[davide]

So, is there anything remaining before we can merge this patch?

[vsk]

I think this patch is complete, it just needs review.

[davide]

OK, I'll try to review it later today. Will you have time to commit this or you want any of us to do it on your behalf?

[aprantl]

Can you watch http://lnt.llvm.org/db_default/v4/nts/117379 (http://lnt.llvm.org/db_default/v4/nts/graph?plot.0=1357.1607042.4 and http://lnt.llvm.org/db_default/v4/nts/graph?plot.0=1357.1607043.4) after landing this?

[davide]

LGTM.

[dstenb]

I do not find the test file on trunk. Also, git log -- test/Transforms/Util/dbg-user-of-aext.ll in the git mirror does not yield anything, so it seems like the file has not previously existed either.

Is it perhaps something that is only available downstream, or am I overlooking something?

[dstenb]

In D51813#1331228, @dstenb wrote:

I do not find the test file on trunk. Also, git log -- test/Transforms/Util/dbg-user-of-aext.ll in the git mirror does not yield anything, so it seems like the file has not previously existed either.

Is it perhaps something that is only available downstream, or am I overlooking something?

But the file looks very similar to test/Transforms/Util/split-bit-piece.ll, and the patch applies cleanly to that file except for some whitespace and comment difference around the @llvm.dbg.declare declaration.

[vsk]

In D51813#1330095, @aprantl wrote:

Can you watch http://lnt.llvm.org/db_default/v4/nts/117379 (http://lnt.llvm.org/db_default/v4/nts/graph?plot.0=1357.1607042.4 and http://lnt.llvm.org/db_default/v4/nts/graph?plot.0=1357.1607043.4) after landing this?

Will do.

And @dstenb I believe I had renamed the test file in this patch, that might have been why it didn't apply cleanly at first.

[vsk]

@aprantl It looks like the percentage of variables with location went up right after this patch landed:

http://lnt.llvm.org/db_default/v4/nts/graph?plot.0=1357.1607043.4
Before (r349213): 63.88%
After (r349223): 63.93%

However, it looks like there was a regression in the percentage of regions covered:

http://lnt.llvm.org/db_default/v4/nts/graph?plot.0=1357.1607042.4
Before (r349213): 52.44%
After (r349223): 51.75%

The first result makes sense to me. Before this patch, if the operand of a [s|z]ext got optimized out, the associated variable location would disappear. After the patch, if the [s|z]ext operand is optimized out, replaceAllDbgUsesWith will make an attempt to preserve the location.

The second result doesn't make sense to me. Why would the percentage of regions covered change as a result of this patch?

llvm/lib/Transforms/Utils/Local.cpp

if (!valueCoversEntireFragment(SI->getValueOperand()->getType(), DII)) {
     // know nothing about the variable's content.
     DV = UndefValue::get(DV->getType());
     if (!LdStHasDebugValue(DIVar, DIExpr, SI))
       Builder.insertDbgValueIntrinsic(DV, DIVar, DIExpr, DII->getDebugLoc(),
                                       SI);
     return;
   }
 
-  // If an argument is zero extended then use argument directly. The ZExt
-  // may be zapped by an optimization pass in future.
-  Argument *ExtendedArg = nullptr;
-  if (ZExtInst *ZExt = dyn_cast<ZExtInst>(SI->getOperand(0)))
-    ExtendedArg = dyn_cast<Argument>(ZExt->getOperand(0));
-  if (SExtInst *SExt = dyn_cast<SExtInst>(SI->getOperand(0)))
-    ExtendedArg = dyn_cast<Argument>(SExt->getOperand(0));
-  if (ExtendedArg) {
-    // If this DII was already describing only a fragment of a variable, ensure
-    // that fragment is appropriately narrowed here.
-    // But if a fragment wasn't used, describe the value as the original
-    // argument (rather than the zext or sext) so that it remains described even
-    // if the sext/zext is optimized away. This widens the variable description,
-    // leaving it up to the consumer to know how the smaller value may be
-    // represented in a larger register.
-    if (auto Fragment = DIExpr->getFragmentInfo()) {
-      unsigned FragmentOffset = Fragment->OffsetInBits;
-      SmallVector<uint64_t, 3> Ops(DIExpr->elements_begin(),
-                                   DIExpr->elements_end() - 3);
-      Ops.push_back(dwarf::DW_OP_LLVM_fragment);
-      Ops.push_back(FragmentOffset);
-      const DataLayout &DL = DII->getModule()->getDataLayout();
-      Ops.push_back(DL.getTypeSizeInBits(ExtendedArg->getType()));
-      DIExpr = Builder.createExpression(Ops);
-    }
-    DV = ExtendedArg;
-  }
   if (!LdStHasDebugValue(DIVar, DIExpr, SI))
     Builder.insertDbgValueIntrinsic(DV, DIVar, DIExpr, DII->getDebugLoc(),
                                     SI);
 }
 
 /// Inserts a llvm.dbg.value intrinsic before a load of an alloca'd value
 /// that has an associated llvm.dbg.declare or llvm.dbg.addr intrinsic.
 void llvm::ConvertDebugDeclareToDebugValue(DbgVariableIntrinsic *DII,

llvm/test/Transforms/Util/dbg-user-of-aext.ll

 ; Checks that llvm.dbg.declare -> llvm.dbg.value conversion utility
-; (here exposed through the SROA) pass, properly inserts bit_piece expressions
-; if it only describes part of the variable.
+; (here exposed through the SROA) pass refers to [s|z]exts of values (as
+; opposed to the operand of a [s|z]ext).
 ; RUN: opt -S -sroa %s | FileCheck %s
 
 ; Built from:
 ; struct foo { bool b; long i; };
 ; void f(bool b, bool expr, foo g) {
 ; }
 ; And modifying the frag dbg.declare to use a fragmented DIExpression (with offset: 0, size: 4)
 ; to test the dbg.declare+fragment case here (as a sanity check).
 
 ; Expect three fragments:
 ; * first starting at bit 0, 8 bits (for the bool)
 ; * second starting at bit 32, 32 bits (for the long)
 ; (this happens to create/demonstrate a gap from bits [7, 32))
 ; * third starting at bit 0, 4 bits (for the added fragment)
 
 ; But also check that a complex expression is not used for a lone bool
 ; parameter. It can reference the register it's in directly without masking off
 ; high bits or anything
 
 ; CHECK: call void @llvm.dbg.value(metadata i8 %g.coerce0, metadata ![[VAR_STRUCT:[0-9]+]], metadata !DIExpression(DW_OP_LLVM_fragment, 0, 8))
 ; CHECK: call void @llvm.dbg.value(metadata i64 %g.coerce1, metadata ![[VAR_STRUCT]], metadata !DIExpression(DW_OP_LLVM_fragment, 32, 64))
-; CHECK: call void @llvm.dbg.value(metadata i1 %b, metadata ![[VAR_BOOL:[0-9]+]], metadata !DIExpression())
-; CHECK: call void @llvm.dbg.value(metadata i1 %frag, metadata ![[VAR_FRAG:[0-9]+]], metadata !DIExpression(DW_OP_LLVM_fragment, 0, 1))
+; CHECK: call void @llvm.dbg.value(metadata i8 %frombool, metadata ![[VAR_BOOL:[0-9]+]], metadata !DIExpression())
+; CHECK: call void @llvm.dbg.value(metadata i8 %frombool1, metadata ![[VAR_FRAG:[0-9]+]], metadata !DIExpression(DW_OP_LLVM_fragment, 0, 4))
 
 %struct.foo = type { i8, i64 }
 
 define void @_Z1fbb3foo(i1 zeroext %b, i1 zeroext %frag, i8 %g.coerce0, i64 %g.coerce1) !dbg !6 {
 entry:
   %g = alloca %struct.foo, align 8
   %b.addr = alloca i8, align 1
   %frag.addr = alloca i8, align 1
   %0 = bitcast %struct.foo* %g to { i8, i64 }*
   %1 = getelementptr inbounds { i8, i64 }, { i8, i64 }* %0, i32 0, i32 0
   store i8 %g.coerce0, i8* %1, align 8
   %2 = getelementptr inbounds { i8, i64 }, { i8, i64 }* %0, i32 0, i32 1
   store i64 %g.coerce1, i64* %2, align 8
   %frombool = zext i1 %b to i8
   store i8 %frombool, i8* %b.addr, align 1
   call void @llvm.dbg.declare(metadata i8* %b.addr, metadata !15, metadata !16), !dbg !17
-  %frombool1 = zext i1 %frag to i8
+  %frombool1 = sext i1 %frag to i8
   store i8 %frombool1, i8* %frag.addr, align 1
   call void @llvm.dbg.declare(metadata i8* %frag.addr, metadata !18, metadata !23), !dbg !19
   call void @llvm.dbg.declare(metadata %struct.foo* %g, metadata !20, metadata !16), !dbg !21
   ret void, !dbg !22
 }
 
+; CHECK: ![[VAR_STRUCT]] = !DILocalVariable(name: "g"
+; CHECK: ![[VAR_BOOL]] = !DILocalVariable(name: "b"
+; CHECK: ![[VAR_FRAG]] = !DILocalVariable(name: "frag"
 
 declare void @llvm.dbg.declare(metadata, metadata, metadata)
 
 !llvm.dbg.cu = !{!0}
 !llvm.module.flags = !{!3, !4}
 !llvm.ident = !{!5}
 
 !0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1, producer: "clang version 5.0.0 (trunk 303077) (llvm/trunk 303098)", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)