[InstCombine] Preserve debug values for eliminable casts

[vsk]

Summary

A cast from A to B is eliminable if its result is casted to C, and if
the pair of casts could just be expressed as a single cast. E.g here,
%c1 is eliminable:

%c1 = zext i16 %A to i32
%c2 = sext i32 %c1 to i64

InstCombine optimizes away eliminable casts. This patch teaches it to
insert a dbg.value intrinsic pointing to the final result, so that local
variables pointing to the eliminable result are preserved.

[JDevlieghere]

lib/Transforms/InstCombine/InstCombineCasts.cpp

275

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. :-)

[aprantl]

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

lib/Transforms/InstCombine/InstCombineCasts.cpp

280

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.

[vsk]

In 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.

lib/Transforms/InstCombine/InstCombineCasts.cpp

275

No particular reason, it's O(1) so it should be OK.

280

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?

[aprantl]

lib/Transforms/InstCombine/InstCombineCasts.cpp

280

I see. If CI guaranteed to be immediately after the old cast, this is indeed correct. Thanks for clarifying.

[vsk]

Thanks for the review :), here's to more -debugify finds!

lib/Transforms/InstCombine/InstCombineCasts.cpp

 //
 //===----------------------------------------------------------------------===//
 
 #include "InstCombineInternal.h"
 #include "llvm/ADT/SetVector.h"
 #include "llvm/Analysis/ConstantFolding.h"
 #include "llvm/Analysis/TargetLibraryInfo.h"
 #include "llvm/IR/DataLayout.h"
+#include "llvm/IR/DIBuilder.h"
 #include "llvm/IR/PatternMatch.h"
 #include "llvm/Support/KnownBits.h"
 using namespace llvm;
 using namespace PatternMatch;
 
 #define DEBUG_TYPE "instcombine"
 
 /// Analyze 'Val', seeing if it is a simple linear expression.

 Instruction *InstCombiner::commonCastTransforms(CastInst &CI) {
   Value *Src = CI.getOperand(0);
 
   // Try to eliminate a cast of a cast.
   if (auto *CSrc = dyn_cast<CastInst>(Src)) {   // A->B->C cast
     if (Instruction::CastOps NewOpc = isEliminableCastPair(CSrc, &CI)) {
       // The first cast (CSrc) is eliminable so we need to fix up or replace
       // the second cast (CI). CSrc will then have a good chance of being dead.
-      return CastInst::Create(NewOpc, CSrc->getOperand(0), CI.getType());
+      auto *Res = CastInst::Create(NewOpc, CSrc->getOperand(0), CI.getType());
+
+      // If the eliminable cast has debug users, insert a debug value after the
+      // cast pointing to the new Value.
+      SmallVector<DbgInfoIntrinsic *, 1> CSrcDbgInsts;
+      findDbgUsers(CSrcDbgInsts, CSrc);
+      if (CSrcDbgInsts.size()) {
+        DIBuilder DIB(*CI.getModule());
+        for (auto *DII : CSrcDbgInsts)
+          DIB.insertDbgValueIntrinsic(
+              Res, DII->getVariable(), DII->getExpression(),
+              DII->getDebugLoc().get(), &*std::next(CI.getIterator()));
+      }
+      return Res;
     }
   }
 
   // If we are casting a select, then fold the cast into the select.
   if (auto *SI = dyn_cast<SelectInst>(Src))
     if (Instruction *NV = FoldOpIntoSelect(CI, SI))
       return NV;
 

test/Transforms/InstCombine/alloca-cast-debuginfo.ll

entry:
   call void @escape(i8* %4), !dbg !34
   %5 = bitcast %struct.Foo* %local to i8*, !dbg !35
   ret void, !dbg !35
 }
 
 ; CHECK-LABEL: define void @f(%struct.Foo* %p)
 ; CHECK: %local = alloca i64, align 8
 ; CHECK: call void @llvm.dbg.declare(metadata i64* %local, metadata !22, metadata !DIExpression())
+; CHECK: [[simplified:%.*]] = bitcast i64* %local to i8*
+; CHECK: call void @llvm.dbg.value(metadata i8* [[simplified]], metadata !22, metadata !DIExpression())
+; CHECK: call void @escape(i8* [[simplified]])
+; CHECK: ret void
 
 declare void @llvm.dbg.declare(metadata, metadata, metadata)
 
 declare void @escape(i8*)
 
 !llvm.dbg.cu = !{!0}
 !llvm.module.flags = !{!6, !7, !8, !9}
 !llvm.ident = !{!10}

test/Transforms/InstCombine/debuginfo-variables.ll

+; RUN: opt < %s -debugify -instcombine -S | FileCheck %s
+
+define i64 @test_sext_zext(i16 %A) {
+; CHECK-LABEL: @test_sext_zext(
+; CHECK-NEXT:  [[C2:%.*]] = zext i16 %A to i64
+; CHECK-NEXT:  call void @llvm.dbg.value(metadata i64 [[C2]], metadata !8, metadata !DIExpression()), !dbg !13
+; CHECK-NEXT:  call void @llvm.dbg.value(metadata i64 [[C2]], metadata !10, metadata !DIExpression()), !dbg !12
+  %c1 = zext i16 %A to i32
+  %c2 = sext i32 %c1 to i64
+  ret i64 %c2
+}
+
+; CHECK: !8 = !DILocalVariable(name: "1", scope: !5, file: !1, line: 1, type: !9)
+; CHECK: !9 = !DIBasicType(name: "ty32", size: 32, encoding: DW_ATE_unsigned)
+; CHECK: !10 = !DILocalVariable(name: "2", scope: !5, file: !1, line: 2, type: !11)
+; CHECK: !11 = !DIBasicType(name: "ty64", size: 64, encoding: DW_ATE_unsigned)
+; CHECK: !12 = !DILocation(line: 2, column: 1, scope: !5)
+; CHECK: !13 = !DILocation(line: 1, column: 1, scope: !5)