[InstCombine] Preserve debug value when simplifying cast-of-select

[vsk]

Summary

InstCombine has a cast transform that matches a cast-of-select:

Orig = cast (Src = select Cond TV FV)

And tries to replace it with a select which has the cast folded in:

NewSel = select Cond (cast TV) (cast FV)

The combiner does RAUW(Orig, NewSel), so any debug values for Orig would
survive the transform. But debug values for Src would be lost.

This patch teaches InstCombine to replace all debug uses of Src with
NewSel (taking care of doing any necessary DIExpression rewriting).

[lebedev.ri]

Judging by the pattern, i would almost guess every single transform in instcombine will need such a fix?
Can this be generalized somehow, fixing the outer place which actually applies replacements?

[vsk]

In D49270#1161213, @lebedev.ri wrote:

Judging by the pattern, i would almost guess every single transform in instcombine will need such a fix?
Can this be generalized somehow, fixing the outer place which actually applies replacements?

There are general debug info salvage calls in InstCombine already (added in r297994 and r318320). These are pretty effective. I've measured that they ~halve the number of debug values dropped in InstCombine during an -O2 build of sqlite3.

However, there is a long tail of bugs that don't have a neat general solution (well, at least not one I can see :).

Fundamentally, after a combine is successfully applied the use counts of some instructions may drop to zero. The question is: what should the debug users of these instructions point to? It wouldn't be conservatively correct to simply point them to the newly-created value. Consider the following contrived combine:

%A = add %x, %x
dbg.value(%A, "my_var_1")

%B = mul %A, 2
dbg.value(%B, "my_var_2")

=>

%B = mul %x, 4
dbg.value(?, "my_var_1")
dbg.value(%B, "my_var_2")

We don't want to emit dbg.value(%B, "my_var_1"), because that would associate the wrong value with the variable "my_var_1". The correct thing to do would be to emit something like dbg.value(%B, "my_var_1", {DW_OP_lit2, DW_OP_div, DW_OP_stack_value}).

While I think there's more yet to be done with applying salvageDebugInfo, I do think we'll need targeted solutions like this one in the long haul.

Edit: Correct the example.

[lebedev.ri]

I would prefer some more general solution, but this does not look awful, too.
You probably want to wait a bit in case others want to comment.

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

Instruction *InstCombiner::commonCastTransforms(CastInst &CI) {
 
   if (auto *Sel = dyn_cast<SelectInst>(Src)) {
     // We are casting a select. Try to fold the cast into the select, but only
     // if the select does not have a compare instruction with matching operand
     // types. Creating a select with operands that are different sizes than its
     // condition may inhibit other folds and lead to worse codegen.
     auto *Cmp = dyn_cast<CmpInst>(Sel->getCondition());
     if (!Cmp || Cmp->getOperand(0)->getType() != Sel->getType())
-      if (Instruction *NV = FoldOpIntoSelect(CI, Sel))
+      if (Instruction *NV = FoldOpIntoSelect(CI, Sel)) {
+        replaceAllDbgUsesWith(*Sel, *NV, CI, DT);
         return NV;
       }
+  }
 
   // If we are casting a PHI, then fold the cast into the PHI.
   if (auto *PN = dyn_cast<PHINode>(Src)) {
     // Don't do this if it would create a PHI node with an illegal type from a
     // legal type.
     if (!Src->getType()->isIntegerTy() || !CI.getType()->isIntegerTy() ||
         shouldChangeType(CI.getType(), Src->getType()))
       if (Instruction *NV = foldOpIntoPhi(CI, PN))

llvm/trunk/test/Transforms/InstCombine/debuginfo-variables.ll

 ; CHECK-NEXT:  call void @escape32(i32 %c1)
 ; CHECK-NEXT:  ret i64 %c2
   %c1 = zext i16 %A to i32
   %c2 = sext i32 %c1 to i64
   call void @escape32(i32 %c1)
   ret i64 %c2
 }
 
+define i32 @test_cast_select(i1 %cond) {
+; CHECK-LABEL: @test_cast_select(
+; CHECK-NEXT:  [[sel:%.*]] = select i1 %cond, i32 3, i32 5
+; CHECK-NEXT:  call void @llvm.dbg.value(metadata i32 [[sel]], {{.*}}, metadata !DIExpression())
+; CHECK-NEXT:  call void @llvm.dbg.value(metadata i32 [[sel]], {{.*}}, metadata !DIExpression())
+; CHECK-NEXT:  ret i32 [[sel]]
+  %sel = select i1 %cond, i16 3, i16 5
+  %cast = zext i16 %sel to i32
+  ret i32 %cast
+}
+
 define void @test_or(i64 %A) {
 ; CHECK-LABEL: @test_or(
 ; CHECK-NEXT:  call void @llvm.dbg.value(metadata i64 %A, {{.*}}, metadata !DIExpression(DW_OP_constu, 256, DW_OP_or, DW_OP_stack_value))
   %1 = or i64 %A, 256
   ret void
 }
 
 define void @test_xor(i32 %A) {