Fix invalid llvm.dbg.declare after instcombine (#56807)

[mgehre-amd]

Summary

Fixes https://github.com/llvm/llvm-project/issues/56807

The problem occurs when the InstCombine pass removes an unnecessary BitCast but does not update the operand of the llvm.dbg.declare intrinsic call. This makes the llvm.dbg.declare have undef as its operand, but the problem can go away if we replace debug uses of the old alloca in the new alloca.

[jmorse]

This looks like the right thing to do, however please upload with context in the diff for review -- see https://llvm.org/docs/DeveloperPolicy.html#making-and-submitting-a-patch , for git you need to add "-U999999" when producing the diff.

[nikolaos-amd]

Updated diff with context.

[jmorse]

This looks good; in particular thanks for writing tests for both code paths. The only thing that needs doing IMO is adding a FileCheck variable capture for the metadata node numbers (see the inline comment) to avoid the test failing if something unrelated in metadata changes.

llvm/test/Transforms/InstCombine/no-undef-bitcast-to-gep-ret.ll

11 ↗

(On Diff #457292)

Tests for metadata nodes ("!7", "!12") shouldn't test the explicit number, because those can change when other parts of LLVM change, and we should avoid spurious test failures. Instead, it's best to capture the metadata numbers and check that they correspond to the right metadata node -- there's an example in D133095.

llvm/test/Transforms/InstCombine/no-undef-bitcast-to-gep.ll

13 ↗

(On Diff #457292)

(Same as with the other test)

[mgehre-amd]

Hey, thanks for the PR!
In the test cases, I think that both of the modified transformations (simplifyAllocaArraySize & PromoteCastOfAllocation) trigger (one after another).

It might be easier to debug those tests in the future when they only test a single transformation each.

[jmorse]

Reverse-ping -- would this be landable for the next release? The test revisions needed are minor.

[mgehre-amd]

Hey @jmorse, I talked to @nikolaos-amd offline and will take over this revision.

[mgehre-amd]

Split tests and remove explicit metadata numbers

[mgehre-amd]

llvm/test/Transforms/InstCombine/dbg-cast-of-allocation.ll

14

Before this PR, the first argument used to be "undef"

llvm/test/Transforms/InstCombine/dbg-simplify-alloca-size.ll

12

Before this PR, the first argument used to be %pixels1.sub.

[jmorse]

Looks good -- the change to alloca-bitcast.ll is suspicious (an undef dbg.assign becoming valued again) but that probably signals an assignment tracking issue. CC @Orlando, what should happen here?

[Orlando]

The first operand should indeed remain undef. Looks like this is caused by a bug I introduced (for assignment tracking only) in DbgVariableIntrinsic::replaceVariableLocationOp - let me get a patch + test together.

[Orlando]

In D133118#3946238, @Orlando wrote:

The first operand should indeed remain undef. Looks like this is caused by a bug I introduced (for assignment tracking only) in DbgVariableIntrinsic::replaceVariableLocationOp - let me get a patch + test together.

This should fix the issue https://reviews.llvm.org/D138561 - with that patch applied the first operand should remain undef.

[mgehre-amd]

Rebase main to update alloca-bitcast.ll

[jmorse]

LGTM, thanks for keeping on this.

llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp

Instruction *InstCombinerImpl::PromoteCastOfAllocation(BitCastInst &CI,
 
   AllocaInst *New = Builder.CreateAlloca(CastElTy, AI.getAddressSpace(), Amt);
   New->setAlignment(AI.getAlign());
   New->takeName(&AI);
   New->setUsedWithInAlloca(AI.isUsedWithInAlloca());
   New->setMetadata(LLVMContext::MD_DIAssignID,
                    AI.getMetadata(LLVMContext::MD_DIAssignID));
 
+  replaceAllDbgUsesWith(AI, *New, *New, DT);
+
   // If the allocation has multiple real uses, insert a cast and change all
   // things that used it to use the new cast.  This will also hack on CI, but it
   // will die soon.
   if (!AI.hasOneUse()) {
     // New is the allocation instruction, pointer typed. AI is the original
     // allocation instruction, also pointer typed. Thus, cast to use is BitCast.
     Value *NewCast = Builder.CreateBitCast(New, AI.getType(), "tmpcast");
     replaceInstUsesWith(AI, NewCast);

llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp

static bool isDereferenceableForAllocaSize(const Value *V, const AllocaInst *AI,
   uint64_t AllocaSize = DL.getTypeStoreSize(AI->getAllocatedType());
   if (!AllocaSize)
     return false;
   return isDereferenceableAndAlignedPointer(V, AI->getAlign(),
                                             APInt(64, AllocaSize), DL);
 }
 
 static Instruction *simplifyAllocaArraySize(InstCombinerImpl &IC,
-                                            AllocaInst &AI) {
+                                            AllocaInst &AI, DominatorTree &DT) {
   // Check for array size of 1 (scalar allocation).
   if (!AI.isArrayAllocation()) {
     // i32 1 is the canonical array size for scalar allocations.
     if (AI.getArraySize()->getType()->isIntegerTy(32))
       return nullptr;
 
     // Canonicalize it.
     return IC.replaceOperand(AI, 0, IC.Builder.getInt32(1));
   }
 
   // Convert: alloca Ty, C - where C is a constant != 1 into: alloca [C x Ty], 1
   if (const ConstantInt *C = dyn_cast<ConstantInt>(AI.getArraySize())) {
     if (C->getValue().getActiveBits() <= 64) {
       Type *NewTy = ArrayType::get(AI.getAllocatedType(), C->getZExtValue());
       AllocaInst *New = IC.Builder.CreateAlloca(NewTy, AI.getAddressSpace(),
                                                 nullptr, AI.getName());
       New->setAlignment(AI.getAlign());
 
+      replaceAllDbgUsesWith(AI, *New, *New, DT);
+
       // Scan to the end of the allocation instructions, to skip over a block of
       // allocas if possible...also skip interleaved debug info
       //
       BasicBlock::iterator It(New);
       while (isa<AllocaInst>(*It) || isa<DbgInfoIntrinsic>(*It))
         ++It;
 
       // Now that I is pointing to the first non-allocation-inst in the block,

 #endif
   WorkMap[&I] = V;
 
   for (Instruction *Workitem : Worklist)
     replace(Workitem);
 }
 
 Instruction *InstCombinerImpl::visitAllocaInst(AllocaInst &AI) {
-  if (auto *I = simplifyAllocaArraySize(*this, AI))
+  if (auto *I = simplifyAllocaArraySize(*this, AI, DT))
     return I;
 
   if (AI.getAllocatedType()->isSized()) {
     // Move all alloca's of zero byte objects to the entry block and merge them
     // together.  Note that we only do this for alloca's, because malloc should
     // allocate and return a unique pointer, even for a zero byte allocation.
     if (DL.getTypeAllocSize(AI.getAllocatedType()).getKnownMinSize() == 0) {
       // For a zero sized alloca there is no point in doing an array allocation.

llvm/test/DebugInfo/Generic/assignment-tracking/instcombine/alloca-bitcast.ll

 ;;   return e;
 ;; }
 ;; $ clang -O2 -c -g reduce.cpp -fno-inline -Xclang -disable-llvm-passes -emit-llvm -S \
 ;;   | opt -passes=declare-to-assign -S
 
 ; CHECK: entry:
 ; CHECK-NEXT: %retval = alloca i64, align 8, !DIAssignID ![[ID:[0-9]+]]
 ; CHECK-NEXT: %tmpcast = bitcast i64* %retval to %struct.c*
-; CHECK-NEXT: call void @llvm.dbg.assign(metadata i1 undef, metadata ![[e:[0-9]+]], metadata !DIExpression(), metadata ![[ID]], metadata %struct.c* %tmpcast, metadata !DIExpression()), !dbg
+; CHECK-NEXT: call void @llvm.dbg.assign(metadata i1 undef, metadata ![[e:[0-9]+]], metadata !DIExpression(), metadata ![[ID]], metadata i64* %retval, metadata !DIExpression()), !dbg
 ; CHECK: ![[e]] = !DILocalVariable(name: "e",
 
 target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
 
 %struct.c = type { i32, i32 }
 
 define dso_local i64 @_Z1dv() !dbg !7 {
 entry:

llvm/test/Transforms/InstCombine/dbg-cast-of-allocation.ll

+; RUN: opt -S --passes=instcombine %s | FileCheck %s
+
+; https://github.com/llvm/llvm-project/issues/56807
+%si16 = type { i8, i8 }
+
+declare void @foo(i16* %pixels)
+
+declare void @llvm.dbg.declare(metadata, metadata, metadata)
+
+; CHECK-LABEL: @toplevel(
+; CHECK:  entry:
+; CHECK-NEXT:    %pixels = alloca i16
+; CHECK-NEXT:    call void @llvm.dbg.declare(metadata i16* %pixels, metadata ![[MD:[0-9]+]], metadata !DIExpression()), !dbg ![[DBG:[0-9]+]]
+; CHECK-NEXT:    call void @foo(i16* nonnull %pixels)
+; CHECK-NEXT:    ret void
+define dso_local void @toplevel() {
+entry:
+  %pixels = alloca %si16
+  call void @llvm.dbg.declare(metadata %si16* %pixels, metadata !11, metadata !DIExpression()), !dbg !12
+  %arraydecay = bitcast %si16* %pixels to i16*
+  call void @foo(i16* nonnull %arraydecay)
+  ret void
+}
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5}
+
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus_14, file: !1, producer: "clang version 11.1.0", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, splitDebugInlining: false, nameTableKind: None)
+!1 = !DIFile(filename: "test.cpp", directory: "/path/to/test_cpp")
+!2 = !{}
+!3 = !{i32 7, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"wchar_size", i32 4}
+!7 = distinct !DISubprogram(name: "toplevel", linkageName: "_Z8toplevelv", scope: !1, file: !1, line: 9, type: !8, scopeLine: 9, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !0, retainedNodes: !2)
+!8 = !DISubroutineType(types: !9)
+!9 = !{!10, !10}
+!10 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+; CHECK: ![[MD]] = !DILocalVariable(name: "pixels"
+!11 = !DILocalVariable(name: "pixels", arg: 1, scope: !7, file: !1, line: 9, type: !10)
+; CHECK: ![[DBG]] = !DILocation(line: 9, column: 16,
+!12 = !DILocation(line: 9, column: 16, scope: !7)

llvm/test/Transforms/InstCombine/dbg-simplify-alloca-size.ll

+; RUN: opt -S --passes=instcombine %s | FileCheck %s
+
+; https://github.com/llvm/llvm-project/issues/56807
+declare void @foo(i8* %pixels)
+
+declare void @llvm.dbg.declare(metadata, metadata, metadata)
+
+; CHECK-LABEL: @toplevel(
+; CHECK:  entry:
+; CHECK-NEXT:    %pixels1 = alloca [3 x i8], align 1
+; CHECK-NEXT:    call void @llvm.dbg.declare(metadata [3 x i8]* %pixels1, metadata ![[MD:[0-9]+]], metadata !DIExpression()), !dbg ![[DBG:[0-9]+]]
+; CHECK-NEXT:    %pixels1.sub = getelementptr inbounds [3 x i8], [3 x i8]* %pixels1, i64 0, i64 0
+; CHECK-NEXT:    call void @foo(i8* nonnull %pixels1.sub)
+; CHECK-NEXT:    ret void
+define dso_local void @toplevel() {
+entry:
+  %pixels = alloca i8, i32 3
+  call void @llvm.dbg.declare(metadata i8* %pixels, metadata !11, metadata !DIExpression()), !dbg !12
+  call void @foo(i8* %pixels)
+  ret void
+}
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus_14, file: !1, producer: "clang version 11.1.0", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, splitDebugInlining: false, nameTableKind: None)
+!1 = !DIFile(filename: "test.cpp", directory: "/path/to/test_cpp")
+!2 = !{}
+!3 = !{i32 7, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"wchar_size", i32 4}
+!7 = distinct !DISubprogram(name: "toplevel", linkageName: "_Z8toplevelv", scope: !1, file: !1, line: 9, type: !8, scopeLine: 9, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !0, retainedNodes: !2)
+!8 = !DISubroutineType(types: !9)
+!9 = !{!10, !10}
+!10 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+; CHECK: ![[MD]] = !DILocalVariable(name: "pixels"
+!11 = !DILocalVariable(name: "pixels", arg: 1, scope: !7, file: !1, line: 9, type: !10)
+; CHECK: ![[DBG]] = !DILocation(line: 9, column: 16,
+!12 = !DILocation(line: 9, column: 16, scope: !7)
+