[InstCombine] Don't change fn signature for calls to declarations (#102596)

nikic · web-flow · commit cc14ecc28133 · 2024-08-12T10:12:00.000+02:00
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.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
