[llvm] [InstCombine] Don't change fn signature for calls to declarations (PR #102596)
Nikita Popov via llvm-commits
llvm-commits at lists.llvm.org
Fri Aug 9 03:27:35 PDT 2024
https://github.com/nikic created https://github.com/llvm/llvm-project/pull/102596
transformConstExprCastCall() implements a number of highly dubious transforms attempting to make a call function type line up with the function type of the called function. Historically, the main value this had was to avoid function type mismatches due to pointer type differences, which is no longer relevant with opaque pointers.
This patch is a step towards reducing the scope of the transform, by applying it only to definitions, not declarations. For declarations, the declared signature might not match the actual function signature, e.g. `void @fn()` is sometimes used as a placeholder for functions with unknown signature. The implementation already bailed out in some cases for declarations, but I think it would be safer to disable the transform entirely.
For the test cases, I've updated some of them to use definitions instead, so that the test coverage is preserved.
>From d6cde7fb8ec505b118759d2509f2d9895dfd1152 Mon Sep 17 00:00:00 2001
From: Nikita Popov <npopov at redhat.com>
Date: Fri, 9 Aug 2024 10:28:09 +0200
Subject: [PATCH] [InstCombine] Don't change fn signature for calls to
declarations
transformConstExprCastCall() implements a number of highly dubious
transforms attempting to make a call function type line up with
the function type of the called function. Historically, the main
value this had was to avoid function type mismatches due to pointer
type differences, which is no longer relevant with opaque pointers.
This patch is a step towards reducing the scope of the transform,
by applying it only to definitions, not declarations. For
declarations, the declared signature might not match the actual
function signature, e.g. `void @fn()` is sometimes used as a
placeholder for functions with unknown signature. The implementation
already bailed out in some cases for declarations, but I think it
would be safer to disable the transform entirely.
For the test cases, I've updated some of them to use definitions
instead, so that the test coverage is preserved.
---
.../InstCombine/InstCombineCalls.cpp | 28 ++++---------------
.../InstCombine/apint-call-cast-target.ll | 15 ++++++++--
.../InstCombine/call-cast-target.ll | 24 ++++++++++++++--
llvm/test/Transforms/InstCombine/call.ll | 6 ++--
.../test/Transforms/InstCombine/opaque-ptr.ll | 10 +++----
5 files changed, 47 insertions(+), 36 deletions(-)
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp b/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
index bf7c91bf363062..a38c990b9ea83a 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
@@ -4106,6 +4106,12 @@ bool InstCombinerImpl::transformConstExprCastCall(CallBase &Call) {
assert(!isa<CallBrInst>(Call) &&
"CallBr's don't have a single point after a def to insert at");
+ // Don't perform the transform for declarations, which may not be fully
+ // accurate. For example, void @foo() is commonly used as a placeholder for
+ // unknown prototypes.
+ if (Callee->isDeclaration())
+ return false;
+
// If this is a call to a thunk function, don't remove the cast. Thunks are
// used to transparently forward all incoming parameters and outgoing return
// values, so it's important to leave the cast in place.
@@ -4142,9 +4148,6 @@ bool InstCombinerImpl::transformConstExprCastCall(CallBase &Call) {
return false; // TODO: Handle multiple return values.
if (!CastInst::isBitOrNoopPointerCastable(NewRetTy, OldRetTy, DL)) {
- if (Callee->isDeclaration())
- return false; // Cannot transform this return value.
-
if (!Caller->use_empty())
return false; // Cannot transform this return value.
}
@@ -4212,25 +4215,6 @@ bool InstCombinerImpl::transformConstExprCastCall(CallBase &Call) {
return false; // Cannot transform to or from byval.
}
- if (Callee->isDeclaration()) {
- // Do not delete arguments unless we have a function body.
- if (FT->getNumParams() < NumActualArgs && !FT->isVarArg())
- return false;
-
- // If the callee is just a declaration, don't change the varargsness of the
- // call. We don't want to introduce a varargs call where one doesn't
- // already exist.
- if (FT->isVarArg() != Call.getFunctionType()->isVarArg())
- return false;
-
- // If both the callee and the cast type are varargs, we still have to make
- // sure the number of fixed parameters are the same or we have the same
- // ABI issues as if we introduce a varargs call.
- if (FT->isVarArg() && Call.getFunctionType()->isVarArg() &&
- FT->getNumParams() != Call.getFunctionType()->getNumParams())
- return false;
- }
-
if (FT->getNumParams() < NumActualArgs && FT->isVarArg() &&
!CallerPAL.isEmpty()) {
// In this case we have more arguments than the new function type, but we
diff --git a/llvm/test/Transforms/InstCombine/apint-call-cast-target.ll b/llvm/test/Transforms/InstCombine/apint-call-cast-target.ll
index d4917621335498..6cb195e3d50927 100644
--- a/llvm/test/Transforms/InstCombine/apint-call-cast-target.ll
+++ b/llvm/test/Transforms/InstCombine/apint-call-cast-target.ll
@@ -4,8 +4,19 @@
target datalayout = "e-p:32:32"
target triple = "i686-pc-linux-gnu"
-declare i32 @main2()
-declare ptr @ctime2(ptr)
+define i32 @main2() {
+; CHECK-LABEL: @main2(
+; CHECK-NEXT: ret i32 0
+;
+ ret i32 0
+}
+
+define ptr @ctime2(ptr %p) {
+; CHECK-LABEL: @ctime2(
+; CHECK-NEXT: ret ptr [[P:%.*]]
+;
+ ret ptr %p
+}
define ptr @ctime(ptr) {
; CHECK-LABEL: @ctime(
diff --git a/llvm/test/Transforms/InstCombine/call-cast-target.ll b/llvm/test/Transforms/InstCombine/call-cast-target.ll
index b9f57dbbd1c303..e5bb929ce92f0c 100644
--- a/llvm/test/Transforms/InstCombine/call-cast-target.ll
+++ b/llvm/test/Transforms/InstCombine/call-cast-target.ll
@@ -16,7 +16,13 @@ entry:
ret i32 %tmp
}
-declare ptr @ctime(ptr)
+define ptr @ctime(ptr %p) {
+; CHECK-LABEL: define ptr @ctime(
+; CHECK-SAME: ptr [[P:%.*]]) {
+; CHECK-NEXT: ret ptr [[P]]
+;
+ ret ptr %p
+}
define internal { i8 } @foo(ptr) {
; CHECK-LABEL: define internal { i8 } @foo(
@@ -39,7 +45,13 @@ entry:
ret void
}
-declare i32 @fn1(i32)
+define i32 @fn1(i32 %x) {
+; CHECK-LABEL: define i32 @fn1(
+; CHECK-SAME: i32 [[X:%.*]]) {
+; CHECK-NEXT: ret i32 [[X]]
+;
+ ret i32 %x
+}
define i32 @test1(ptr %a) {
; CHECK-LABEL: define i32 @test1(
@@ -116,7 +128,13 @@ define i1 @test5() {
ret i1 %6
}
-declare void @bundles_callee(i32)
+define void @bundles_callee(i32) {
+; CHECK-LABEL: define void @bundles_callee(
+; CHECK-SAME: i32 [[TMP0:%.*]]) {
+; CHECK-NEXT: ret void
+;
+ ret void
+}
define void @bundles() {
; CHECK-LABEL: define void @bundles() {
diff --git a/llvm/test/Transforms/InstCombine/call.ll b/llvm/test/Transforms/InstCombine/call.ll
index 52de13ba06b359..333b3c499aba0a 100644
--- a/llvm/test/Transforms/InstCombine/call.ll
+++ b/llvm/test/Transforms/InstCombine/call.ll
@@ -110,7 +110,7 @@ declare i32 @test6a(i32)
define i32 @test6() {
; CHECK-LABEL: define i32 @test6() {
-; CHECK-NEXT: [[X:%.*]] = call i32 @test6a(i32 0)
+; CHECK-NEXT: [[X:%.*]] = call i32 @test6a()
; CHECK-NEXT: ret i32 [[X]]
;
%X = call i32 @test6a( )
@@ -141,12 +141,12 @@ declare void @test8a()
define ptr @test8() personality ptr @__gxx_personality_v0 {
; CHECK-LABEL: define ptr @test8() personality ptr @__gxx_personality_v0 {
; CHECK-NEXT: invoke void @test8a()
-; CHECK-NEXT: to label [[INVOKE_CONT:%.*]] unwind label [[TRY_HANDLER:%.*]]
+; CHECK-NEXT: to label [[INVOKE_CONT:%.*]] unwind label [[TRY_HANDLER:%.*]]
; CHECK: invoke.cont:
; CHECK-NEXT: unreachable
; CHECK: try.handler:
; CHECK-NEXT: [[EXN:%.*]] = landingpad { ptr, i32 }
-; CHECK-NEXT: cleanup
+; CHECK-NEXT: cleanup
; CHECK-NEXT: ret ptr null
;
; Don't turn this into "unreachable": the callee and caller don't agree in
diff --git a/llvm/test/Transforms/InstCombine/opaque-ptr.ll b/llvm/test/Transforms/InstCombine/opaque-ptr.ll
index df85547f56d74f..a52a07389baee2 100644
--- a/llvm/test/Transforms/InstCombine/opaque-ptr.ll
+++ b/llvm/test/Transforms/InstCombine/opaque-ptr.ll
@@ -743,8 +743,7 @@ declare void @call_byval(i64, ptr byval(i64))
define void @call_cast_ptr_to_int(ptr %p) {
; CHECK-LABEL: @call_cast_ptr_to_int(
-; CHECK-NEXT: [[TMP1:%.*]] = ptrtoint ptr [[P:%.*]] to i64
-; CHECK-NEXT: call void @call_i64(i64 [[TMP1]])
+; CHECK-NEXT: call void @call_i64(ptr [[P:%.*]])
; CHECK-NEXT: ret void
;
call void @call_i64(ptr %p)
@@ -753,8 +752,7 @@ define void @call_cast_ptr_to_int(ptr %p) {
define void @call_cast_byval(ptr %p, ptr %p2) {
; CHECK-LABEL: @call_cast_byval(
-; CHECK-NEXT: [[TMP1:%.*]] = ptrtoint ptr [[P:%.*]] to i64
-; CHECK-NEXT: call void @call_byval(i64 [[TMP1]], ptr byval(double) [[P2:%.*]])
+; CHECK-NEXT: call void @call_byval(ptr [[P:%.*]], ptr byval(double) [[P2:%.*]])
; CHECK-NEXT: ret void
;
call void @call_byval(ptr %p, ptr byval(double) %p2)
@@ -765,8 +763,8 @@ declare float @fmodf(float, float)
define i32 @const_fold_call_with_func_type_mismatch() {
; CHECK-LABEL: @const_fold_call_with_func_type_mismatch(
-; CHECK-NEXT: [[V:%.*]] = call float @fmodf(float 0x40091EB860000000, float 2.000000e+00)
-; CHECK-NEXT: ret i32 1066527622
+; CHECK-NEXT: [[V:%.*]] = call i32 @fmodf(float 0x40091EB860000000, float 2.000000e+00)
+; CHECK-NEXT: ret i32 [[V]]
;
%v = call i32 @fmodf(float 0x40091EB860000000, float 2.000000e+00)
ret i32 %v
More information about the llvm-commits
mailing list