[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 ↗

(On Diff #131551)

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 ↗

(On Diff #131551)

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 ↗

(On Diff #131551)

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

280 ↗

(On Diff #131551)

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 ↗

(On Diff #131551)

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!

llvm/trunk/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;
 

llvm/trunk/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}

llvm/trunk/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)