[InstCombine] Call InstSimplify for cast instructions

[nikic]

InstCombine currently fails to call into InstSimplify for cast instructions. I noticed this because the transform from #98649 can be triggered via -passes=instsimplify but not -passes=instcombine, which is not supposed to happen.

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Author: Nikita Popov (nikic)

Changes

InstCombine currently fails to call into InstSimplify for cast instructions. I noticed this because the transform from #98649 can be triggered via -passes=instsimplify but not -passes=instcombine, which is not supposed to happen.

---

Full diff: https://github.com/llvm/llvm-project/pull/162849.diff

3 Files Affected:

• (modified) llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp (+3-3)
• (modified) llvm/test/Transforms/InstCombine/cast-set-preserve-signed-dbg-val.ll (+1-3)
• (modified) llvm/test/Transforms/InstCombine/ptr-int-cast.ll (+11)

diff --git a/llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp b/llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp
index 4c9b10a094981..cdc559b489e9d 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp
@@ -156,9 +156,9 @@ Instruction *InstCombinerImpl::commonCastTransforms(CastInst &CI) {
   Value *Src = CI.getOperand(0);
   Type *Ty = CI.getType();
 
-  if (auto *SrcC = dyn_cast<Constant>(Src))
-    if (Constant *Res = ConstantFoldCastOperand(CI.getOpcode(), SrcC, Ty, DL))
-      return replaceInstUsesWith(CI, Res);
+  if (Value *Res =
+          simplifyCastInst(CI.getOpcode(), Src, Ty, SQ.getWithInstruction(&CI)))
+    return replaceInstUsesWith(CI, Res);
 
   // Try to eliminate a cast of a cast.
   if (auto *CSrc = dyn_cast<CastInst>(Src)) {   // A->B->C cast
diff --git a/llvm/test/Transforms/InstCombine/cast-set-preserve-signed-dbg-val.ll b/llvm/test/Transforms/InstCombine/cast-set-preserve-signed-dbg-val.ll
index 7cc4446f1038b..ad45d1e3e3e4f 100644
--- a/llvm/test/Transforms/InstCombine/cast-set-preserve-signed-dbg-val.ll
+++ b/llvm/test/Transforms/InstCombine/cast-set-preserve-signed-dbg-val.ll
@@ -11,10 +11,8 @@ define i16 @test5(i16 %A) !dbg !34 {
   call void @llvm.dbg.value(metadata i32 %C, metadata !37, metadata !DIExpression()), !dbg !41
 
   ; Preserve the dbg.value for the DCE'd 32-bit 'and'.
-  ;
-  ; The high 16 bits of the original 'and' require sign-extending the new 16-bit and:
   ; CHECK-NEXT: #dbg_value(i16 [[and]], [[C:![0-9]+]],
-  ; CHECK-SAME:    !DIExpression(DW_OP_LLVM_convert, 16, DW_ATE_signed, DW_OP_LLVM_convert, 32, DW_ATE_signed, DW_OP_stack_value)
+  ; CHECK-SAME:    !DIExpression(DW_OP_LLVM_convert, 16, DW_ATE_unsigned, DW_OP_LLVM_convert, 32, DW_ATE_unsigned, DW_OP_stack_value)
 
   %D = trunc i32 %C to i16, !dbg !42
   call void @llvm.dbg.value(metadata i16 %D, metadata !38, metadata !DIExpression()), !dbg !42
diff --git a/llvm/test/Transforms/InstCombine/ptr-int-cast.ll b/llvm/test/Transforms/InstCombine/ptr-int-cast.ll
index 69b8f6953d61e..82ecbd41e50a7 100644
--- a/llvm/test/Transforms/InstCombine/ptr-int-cast.ll
+++ b/llvm/test/Transforms/InstCombine/ptr-int-cast.ll
@@ -86,3 +86,14 @@ define <4 x ptr> @test7(<4 x i128> %arg) nounwind {
   %p1 = inttoptr <4 x i128> %arg to <4 x ptr>
   ret <4 x ptr> %p1
 }
+
+define i64 @ptrtoint_gep_sub(ptr %ptr, i64 %end.addr) {
+; CHECK-LABEL: @ptrtoint_gep_sub(
+; CHECK-NEXT:    ret i64 [[END_ADDR:%.*]]
+;
+  %ptr.addr = ptrtoint ptr %ptr to i64
+  %size = sub i64 %end.addr, %ptr.addr
+  %end = getelementptr i8, ptr %ptr, i64 %size
+  %end.addr2 = ptrtoint ptr %end to i64
+  ret i64 %end.addr2
+}

[nikic]

My reading of this DIExpression is that we're first extending to 32-bit and then truncating to 16-bit, in which case the signedness does not matter. But I'm not confident on that reading...

The difference in behavior is that we're directly removing a cast pair instead of converting it into a bitcast first, which has some special handling here, though I have no idea what it's doing: https://github.com/nikic/llvm-project/blob/087ad0bea2e991d6c6b546170994cb2d172a6261/llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp#L169-L171

[OCHyams]

Hmm this is interesting. Salvaging (salvageDebugInfo) a trunk or s|zext adds a convert which is signed if the instruction isa<SExtInst>, whereas replaceAllDbgUsesWith applies the signedness of the variable itself (ignores the instruction) to the convert.

It's not immediately clear to me which is best.

Either way, I think we should consider that separately and I don't think it should block this change.

[jmorse]

Uurgh. This is similar to #151225, there's no strong typing of dbg.value operands, and so the transformation of IR Values sometimes isn't observed by any debug-info code (or: isn't always accounted for).

I don't think we really have a solid answer for what should happen here: is the sign-extend to produce a Value for an unsigned variable meaningful, or just an artifact of optimisation? CC @slinder1 on the topic of typed DIExpressions again.

IMO we're best off just letting this slide for now (land the patch, raise an issue?) because it's just as likely that the existing behaviour is unexpected as the new behaviour, and it shouldn't block better optimisations.

[OCHyams]

I'm not super familiar with the code but the change makes sense, LGTM. The debug info change is curious, but I don't think it should block this

[OCHyams]

Hmm this is interesting. Salvaging (salvageDebugInfo) a trunk or s|zext adds a convert which is signed if the instruction isa<SExtInst>, whereas replaceAllDbgUsesWith applies the signedness of the variable itself (ignores the instruction) to the convert.

It's not immediately clear to me which is best.

Either way, I think we should consider that separately and I don't think it should block this change.