[MLIR][LLVMIR] Import calls with mismatching signature as indirect call #135895

bcardosolopes · 2025-04-16T01:40:02Z

LLVM IR currently accepts:

define void @incompatible_call_and_callee_types() {
  call void @callee(i64 0)
  ret void
}

define void @callee({ptr, i64}, i32) {
  ret void
}

This currently fails to import. Even though these constructs are dangerous and probably indicate some ODR violation (or optimization bug), they are "valid" and should be imported into LLVM IR dialect. This PR implements that by using an indirect call to represent it. Translation already works nicely and outputs the same source llvm IR file.

The error is now a warning, the tests in
mlir/test/Target/LLVMIR/Import/import-failure.ll already use CHECK lines, so no need to add extra diagnostic tests.

llvmbot · 2025-04-16T01:40:51Z

@llvm/pr-subscribers-mlir-llvm

@llvm/pr-subscribers-mlir

Author: Bruno Cardoso Lopes (bcardosolopes)

Changes

LLVM IR currently accepts:

define void @<!-- -->incompatible_call_and_callee_types() {
  call void @<!-- -->callee(i64 0)
  ret void
}

define void @<!-- -->callee({ptr, i64}, i32) {
  ret void
}

This currently fails to import. Even though these constructs are dangerous and probably indicate some ODR violation (or optimization bug), they are "valid" and should be imported into LLVM IR dialect. This PR implements that by using an indirect call to represent it. Translation already works nicely and outputs the same source llvm IR file.

The error is now a warning, the tests in
mlir/test/Target/LLVMIR/Import/import-failure.ll already use CHECK lines, so no need to add extra diagnostic tests.

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

3 Files Affected:

(modified) mlir/include/mlir/Target/LLVMIR/ModuleImport.h (+6-3)
(modified) mlir/lib/Target/LLVMIR/ModuleImport.cpp (+46-16)
(modified) mlir/test/Target/LLVMIR/Import/instructions.ll (+16)

diff --git a/mlir/include/mlir/Target/LLVMIR/ModuleImport.h b/mlir/include/mlir/Target/LLVMIR/ModuleImport.h
index 3dc848c413905..7b01a96026413 100644
--- a/mlir/include/mlir/Target/LLVMIR/ModuleImport.h
+++ b/mlir/include/mlir/Target/LLVMIR/ModuleImport.h
@@ -362,9 +362,12 @@ class ModuleImport {
   /// Converts the callee's function type. For direct calls, it converts the
   /// actual function type, which may differ from the called operand type in
   /// variadic functions. For indirect calls, it converts the function type
-  /// associated with the call instruction. Returns failure when the call and
-  /// the callee are not compatible or when nested type conversions failed.
-  FailureOr<LLVMFunctionType> convertFunctionType(llvm::CallBase *callInst);
+  /// associated with the call instruction. When the call and the callee are not
+  /// compatible (or when nested type conversions failed), emit a warning but
+  /// attempt translation using a bitcast and an indirect call (in order
+  /// represent valid and verified LLVM IR).
+  FailureOr<LLVMFunctionType> convertFunctionType(llvm::CallBase *callInst,
+                                                  Value &castResult);
   /// Returns the callee name, or an empty symbol if the call is not direct.
   FlatSymbolRefAttr convertCalleeName(llvm::CallBase *callInst);
   /// Converts the parameter and result attributes attached to `func` and adds
diff --git a/mlir/lib/Target/LLVMIR/ModuleImport.cpp b/mlir/lib/Target/LLVMIR/ModuleImport.cpp
index 187f1bdf7af6e..c1f2f378c87fe 100644
--- a/mlir/lib/Target/LLVMIR/ModuleImport.cpp
+++ b/mlir/lib/Target/LLVMIR/ModuleImport.cpp
@@ -1668,8 +1668,8 @@ ModuleImport::convertCallOperands(llvm::CallBase *callInst,
 /// Checks if `callType` and `calleeType` are compatible and can be represented
 /// in MLIR.
 static LogicalResult
-verifyFunctionTypeCompatibility(LLVMFunctionType callType,
-                                LLVMFunctionType calleeType) {
+checkFunctionTypeCompatibility(LLVMFunctionType callType,
+                               LLVMFunctionType calleeType) {
   if (callType.getReturnType() != calleeType.getReturnType())
     return failure();
 
@@ -1695,7 +1695,7 @@ verifyFunctionTypeCompatibility(LLVMFunctionType callType,
 }
 
 FailureOr<LLVMFunctionType>
-ModuleImport::convertFunctionType(llvm::CallBase *callInst) {
+ModuleImport::convertFunctionType(llvm::CallBase *callInst, Value &castResult) {
   auto castOrFailure = [](Type convertedType) -> FailureOr<LLVMFunctionType> {
     auto funcTy = dyn_cast_or_null<LLVMFunctionType>(convertedType);
     if (!funcTy)
@@ -1718,11 +1718,17 @@ ModuleImport::convertFunctionType(llvm::CallBase *callInst) {
   if (failed(calleeType))
     return failure();
 
-  // Compare the types to avoid constructing illegal call/invoke operations.
-  if (failed(verifyFunctionTypeCompatibility(*callType, *calleeType))) {
+  // Compare the types, if they are not compatible, avoid illegal call/invoke
+  // operations by casting to the callsite type and issuing an indirect call.
+  // LLVM IR currently supports this usage.
+  if (failed(checkFunctionTypeCompatibility(*callType, *calleeType))) {
     Location loc = translateLoc(callInst->getDebugLoc());
-    return emitError(loc) << "incompatible call and callee types: " << *callType
-                          << " and " << *calleeType;
+    FlatSymbolRefAttr calleeSym = convertCalleeName(callInst);
+    castResult = builder.create<LLVM::AddressOfOp>(
+        loc, LLVM::LLVMPointerType::get(context), calleeSym);
+    emitWarning(loc) << "incompatible call and callee types: " << *callType
+                     << " and " << *calleeType;
+    return callType;
   }
 
   return calleeType;
@@ -1839,16 +1845,29 @@ LogicalResult ModuleImport::convertInstruction(llvm::Instruction *inst) {
                 /*operand_attrs=*/nullptr)
             .getOperation();
       }
-      FailureOr<LLVMFunctionType> funcTy = convertFunctionType(callInst);
+      Value castResult;
+      FailureOr<LLVMFunctionType> funcTy =
+          convertFunctionType(callInst, castResult);
       if (failed(funcTy))
         return failure();
 
-      FlatSymbolRefAttr callee = convertCalleeName(callInst);
-      auto callOp = builder.create<CallOp>(loc, *funcTy, callee, *operands);
+      FlatSymbolRefAttr callee = nullptr;
+      // If no cast is needed, use the original callee name. Otherwise patch
+      // operands to include the indirect call target. Build indirect call by
+      // passing using a nullptr `callee`.
+      if (!castResult)
+        callee = convertCalleeName(callInst);
+      else
+        operands->insert(operands->begin(), castResult);
+      CallOp callOp = builder.create<CallOp>(loc, *funcTy, callee, *operands);
+
       if (failed(convertCallAttributes(callInst, callOp)))
         return failure();
-      // Handle parameter and result attributes.
-      convertParameterAttributes(callInst, callOp, builder);
+
+      // Handle parameter and result attributes. Don't bother if there's a
+      // type mismatch.
+      if (!castResult)
+        convertParameterAttributes(callInst, callOp, builder);
       return callOp.getOperation();
     }();
 
@@ -1913,11 +1932,20 @@ LogicalResult ModuleImport::convertInstruction(llvm::Instruction *inst) {
                                  unwindArgs)))
       return failure();
 
-    FailureOr<LLVMFunctionType> funcTy = convertFunctionType(invokeInst);
+    Value castResult;
+    FailureOr<LLVMFunctionType> funcTy =
+        convertFunctionType(invokeInst, castResult);
     if (failed(funcTy))
       return failure();
 
-    FlatSymbolRefAttr calleeName = convertCalleeName(invokeInst);
+    FlatSymbolRefAttr calleeName = nullptr;
+    // If no cast is needed, use the original callee name. Otherwise patch
+    // operands to include the indirect call target. Build indirect call by
+    // passing using a nullptr `callee`.
+    if (!castResult)
+      calleeName = convertCalleeName(invokeInst);
+    else
+      operands->insert(operands->begin(), castResult);
 
     // Create the invoke operation. Normal destination block arguments will be
     // added later on to handle the case in which the operation result is
@@ -1929,8 +1957,10 @@ LogicalResult ModuleImport::convertInstruction(llvm::Instruction *inst) {
     if (failed(convertInvokeAttributes(invokeInst, invokeOp)))
       return failure();
 
-    // Handle parameter and result attributes.
-    convertParameterAttributes(invokeInst, invokeOp, builder);
+    // Handle parameter and result attributes. Don't bother if there's a
+    // type mismatch.
+    if (!castResult)
+      convertParameterAttributes(invokeInst, invokeOp, builder);
 
     if (!invokeInst->getType()->isVoidTy())
       mapValue(inst, invokeOp.getResults().front());
diff --git a/mlir/test/Target/LLVMIR/Import/instructions.ll b/mlir/test/Target/LLVMIR/Import/instructions.ll
index c294e1b34f9bb..ef3c6430b7152 100644
--- a/mlir/test/Target/LLVMIR/Import/instructions.ll
+++ b/mlir/test/Target/LLVMIR/Import/instructions.ll
@@ -720,3 +720,19 @@ bb2:
 declare void @g(...)
 
 declare i32 @__gxx_personality_v0(...)
+
+; // -----
+
+; CHECK-LABEL: llvm.func @incompatible_call_and_callee_types
+define void @incompatible_call_and_callee_types() {
+  ; CHECK: %[[CST:.*]] = llvm.mlir.constant(0 : i64) : i64
+  ; CHECK: %[[TARGET:.*]] = llvm.mlir.addressof @callee : !llvm.ptr
+  ; CHECK: llvm.call %[[TARGET]](%[[CST]]) : !llvm.ptr, (i64) -> ()
+  call void @callee(i64 0)
+  ; CHECK: llvm.return
+  ret void
+}
+
+define void @callee({ptr, i64}, i32) {
+  ret void
+}

Dinistro · 2025-04-16T07:38:09Z

I'm somewhat certain that LLVM specifies this is illegal, but the verifier doesn't trigger. I'm currently at EuroLLVM, so I can only give more context tomorrow.

bcardosolopes · 2025-04-16T18:10:30Z

I'm somewhat certain that LLVM specifies this is illegal, but the verifier doesn't trigger. I'm currently at EuroLLVM, so I can only give more context tomorrow.

If you can share some extra context that'd be great, couldn't find much discussions around it. It does looks like a bug, and seems to be around long enough that several people might be "relying" on it, which is unfortunate.

Before writing this PR I ran the LLVM's verifier under the debugger and note that at the place LLVM verifier does this check, FTy comes from Call.getFunctionType(), which isn't really about the actual function type. Last time this was changed was back in 2020, for what looks more like a refactor, not really a functional change (https://reviews.llvm.org/D56143).

bcardosolopes · 2025-04-16T23:13:40Z

I actually tried to fix the LLVM verifier (see this draft PR: #136052) but got around 124 check-llvm failures. I think there's enough evidence that even though this is undefined behavior, its one that the world depends on. Looks like the best that can be done right now is use opt -passes=lint to get a hint, see https://godbolt.org/z/q19vhbq6K.

IMO the llvm importer should support this until LLVM proper is fixed (if ever). Best we can do right now is to issue a warning (moral equivalent of -passes=lint, I guess). Unfortunately I don't have the time to go chase that battle in the LLVM side. Happy to keep this as a downstream patch if MLIR doesn't want it.

joker-eph

I consider this a verifier bug in LLVM.

This is actually a regression from a migration to opaque pointer: #63484

Dinistro

As Mehdi points out, this is UB and causes various issues. How to properly deal with this on the LLVM side is unclear to me, though.

Note that supporting this leads to various invariant violations and verifier problems in MLIR, thus supporting this broken input is undesired for many users. If users somehow want to support this, they can implement an LLVM pass that pre-processes the IR accordingly, for example constructing a wrapper function to bypass the incompatibility.

bcardosolopes · 2025-04-17T18:15:10Z

Thanks for the context and feedback guys!

bcardosolopes · 2025-04-21T23:40:26Z

For better or worse, LangRef now reflects the status quo: #136189.

My understanding is that this PR now is valid w.r.t LLVM rules - @joker-eph considers this a verifier bug, but that's not what the LLVM maintainers consider (see discussions in the PR above).

It might be in the best interest of MLIR LLVM dialect to follow the langref - If this PR should take a different approach, happy to make it happen.

joker-eph · 2025-04-22T11:24:28Z

What is blocked on this right now? Where do we actually need to support this?

Dinistro · 2025-04-23T05:58:22Z

What is blocked on this right now? Where do we actually need to support this?

From what I understood, this is blocking the import of some "valid" LLVM IR. In my experience this IR always stems from illegal input, but given that this kind of IR was now incorporated into the LangRef, we probably should support or at least handle it.

That being said, we should first consider what this implies for existing code, as things like inlining strongly assumes sane types. IIRC, the original import converted incompatible calls into indirect calls to circumvent this, but that is also a hack IMO.

gysit · 2025-04-23T06:06:07Z

IIRC, the original import converted incompatible calls into indirect calls to circumvent this, but that is also a hack IMO.

Right there is an LLVM helper function for this that was used by the import and that is also used by LLVM itself to avoid inlining / optimizations for such calls.

joker-eph · 2025-04-23T10:09:43Z

The resolution from the LLVM IR side was to consider that "if the types of the arguments mismatch the type of the callee, then it is an indirect call".
The main discrepancy is a MLIR symbol isn't a pointer, while LLVM decided that function are just opaque pointers.
So handling this in the import to make such cases MLIR indirect calls seems to match better LLVM IR than disabling the verifier.

Our documentation for our calls states:

The call instruction supports both direct and indirect calls. Direct calls start with a function name (@-prefixed) and 
indirect calls start with an SSA value (%-prefixed). The direct callee, if present, is stored as a function attribute callee. 
For indirect calls, the callee is of !llvm.ptr type and is stored as the first value in callee_operands.

bcardosolopes · 2025-04-28T22:22:42Z

In my experience this IR always stems from illegal input

My experience differs, we have production code that contains such IR - whether that's good or bad it's orthogonal to the point here.

The resolution from the LLVM IR side was to consider that "if the types of the arguments mismatch the type of the callee, then it is an indirect call".

This PR implements a very similar approach.

IIRC, the original import converted incompatible calls into indirect calls to circumvent this, but that is also a hack IMO.

What do you suggest instead if we are to do this upstream?

Right there is an LLVM helper function for this that was used by the import and that is also used by LLVM itself to avoid inlining / optimizations for such calls.

Interesting, mind sharing the name?

Reviewers: it's not clear from the comments whether there's consensus to move ahead with this PR (like state above, I'm happy to implement any specific solution if this isn't good enough). I'd love a clear answer - I don't want to waste anyone's time if this is leading nowhere.

joker-eph · 2025-04-28T22:30:20Z

This PR implements a very similar approach.

Oh that's fine with me, I thought from the title you were just relaxing the IR verifier instead.

bcardosolopes · 2025-04-28T23:16:19Z

Oh that's fine with me, I thought from the title you were just relaxing the IR verifier instead.

Good point, the title doesn't help with the intent. Thanks for the fast feedback Mehdi

gysit

LGTM modulo some nits (if they make sense).

mlir/lib/Target/LLVMIR/ModuleImport.cpp

mlir/include/mlir/Target/LLVMIR/ModuleImport.h

mlir/lib/Target/LLVMIR/ModuleImport.cpp

mlir/include/mlir/Target/LLVMIR/ModuleImport.h

@callee

LLVM IR currently [accepts](https://godbolt.org/z/nqnEsW1ja): ``` define void @incompatible_call_and_callee_types() { call void @callee(i64 0) ret void } define void @callee({ptr, i64}, i32) { ret void } ``` This currently fails to import. Even though these constructs are dangerous and probably indicate some ODR violation (or optimization bug), they are "valid" and should be imported into LLVM IR dialect. This PR implements that by using an indirect call to represent it. Translation already works nicely and outputs the same source llvm IR file. The error is now a warning, the tests in `mlir/test/Target/LLVMIR/Import/import-failure.ll` already use `CHECK` lines, so no need to add extra diagnostic tests.

mlir/include/mlir/Target/LLVMIR/ModuleImport.h

mlir/lib/Target/LLVMIR/ModuleImport.cpp

Dinistro

LGTM % final location nits. Thanks for pushing on this 🙏

mlir/lib/Target/LLVMIR/ModuleImport.cpp

@callee

…ll (llvm#135895) LLVM IR currently [accepts](https://godbolt.org/z/nqnEsW1ja): ``` define void @incompatible_call_and_callee_types() { call void @callee(i64 0) ret void } define void @callee({ptr, i64}, i32) { ret void } ``` This currently fails to import. Even though these constructs are dangerous and probably indicate some ODR violation (or optimization bug), they are "valid" and should be imported into LLVM IR dialect. This PR implements that by using an indirect call to represent it. Translation already works nicely and outputs the same source llvm IR file. The error is now a warning, the tests in `mlir/test/Target/LLVMIR/Import/import-failure.ll` already use `CHECK` lines, so no need to add extra diagnostic tests.

@callee

…ll (llvm#135895) LLVM IR currently [accepts](https://godbolt.org/z/nqnEsW1ja): ``` define void @incompatible_call_and_callee_types() { call void @callee(i64 0) ret void } define void @callee({ptr, i64}, i32) { ret void } ``` This currently fails to import. Even though these constructs are dangerous and probably indicate some ODR violation (or optimization bug), they are "valid" and should be imported into LLVM IR dialect. This PR implements that by using an indirect call to represent it. Translation already works nicely and outputs the same source llvm IR file. The error is now a warning, the tests in `mlir/test/Target/LLVMIR/Import/import-failure.ll` already use `CHECK` lines, so no need to add extra diagnostic tests.

@callee

…ll (llvm#135895) (#40) LLVM IR currently [accepts](https://godbolt.org/z/nqnEsW1ja): ``` define void @incompatible_call_and_callee_types() { call void @callee(i64 0) ret void } define void @callee({ptr, i64}, i32) { ret void } ``` This currently fails to import. Even though these constructs are dangerous and probably indicate some ODR violation (or optimization bug), they are "valid" and should be imported into LLVM IR dialect. This PR implements that by using an indirect call to represent it. Translation already works nicely and outputs the same source llvm IR file. The error is now a warning, the tests in `mlir/test/Target/LLVMIR/Import/import-failure.ll` already use `CHECK` lines, so no need to add extra diagnostic tests. Co-authored-by: Bruno Cardoso Lopes <[email protected]>

bcardosolopes requested review from gysit, Dinistro and xlauko April 16, 2025 01:40

llvmbot added mlir:llvm mlir labels Apr 16, 2025

bcardosolopes force-pushed the mlir-llvm-incompatible-call branch from dad274b to 4992f78 Compare April 16, 2025 01:41

joker-eph requested changes Apr 16, 2025

View reviewed changes

Dinistro reviewed Apr 17, 2025

View reviewed changes

bcardosolopes mentioned this pull request Apr 17, 2025

Verifier regression for caller/callee after enabling opaque pointers #63484

Open

joker-eph approved these changes Apr 28, 2025

View reviewed changes

joker-eph changed the title ~~[MLIR][LLVMIR] Relax mismatching calls~~ [MLIR][LLVMIR] Import calls with mismatching signature as indirect call Apr 28, 2025

gysit approved these changes Apr 29, 2025

View reviewed changes

bcardosolopes force-pushed the mlir-llvm-incompatible-call branch from 4992f78 to b72c3e3 Compare May 2, 2025 00:27

bcardosolopes added 2 commits May 1, 2025 17:47

address review

6fce89c

update one more comment

2870639

Dinistro reviewed May 2, 2025

View reviewed changes

mlir/include/mlir/Target/LLVMIR/ModuleImport.h Outdated Show resolved Hide resolved

mlir/lib/Target/LLVMIR/ModuleImport.cpp Show resolved Hide resolved

mlir/lib/Target/LLVMIR/ModuleImport.cpp Show resolved Hide resolved

bcardosolopes added 3 commits May 2, 2025 12:36

make the warning check more explicit

bd812d0

cleanup convertFunctionType and related usage

7f20837

Merge branch 'main' into mlir-llvm-incompatible-call

9584b33

Dinistro approved these changes May 5, 2025

View reviewed changes

mlir/lib/Target/LLVMIR/ModuleImport.cpp Outdated Show resolved Hide resolved

mlir/lib/Target/LLVMIR/ModuleImport.cpp Outdated Show resolved Hide resolved

bcardosolopes added 2 commits May 5, 2025 11:21

Use existing loc

2fa593e

Merge branch 'main' into mlir-llvm-incompatible-call

5f3dff8

bcardosolopes merged commit 7682f66 into llvm:main May 5, 2025
11 checks passed

[MLIR][LLVMIR] Import calls with mismatching signature as indirect call #135895

[MLIR][LLVMIR] Import calls with mismatching signature as indirect call #135895

Uh oh!

Conversation

bcardosolopes commented Apr 16, 2025

Uh oh!

llvmbot commented Apr 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Dinistro commented Apr 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bcardosolopes commented Apr 16, 2025

Uh oh!

bcardosolopes commented Apr 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joker-eph left a comment

Choose a reason for hiding this comment

Uh oh!

Dinistro left a comment

Choose a reason for hiding this comment

Uh oh!

bcardosolopes commented Apr 17, 2025

Uh oh!

bcardosolopes commented Apr 21, 2025

Uh oh!

joker-eph commented Apr 22, 2025

Uh oh!

Dinistro commented Apr 23, 2025

Uh oh!

gysit commented Apr 23, 2025

Uh oh!

joker-eph commented Apr 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bcardosolopes commented Apr 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joker-eph commented Apr 28, 2025

Uh oh!

bcardosolopes commented Apr 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gysit left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Dinistro left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

llvmbot commented Apr 16, 2025 •

edited

Loading

Dinistro commented Apr 16, 2025 •

edited

Loading

bcardosolopes commented Apr 16, 2025 •

edited

Loading

joker-eph commented Apr 23, 2025 •

edited

Loading

bcardosolopes commented Apr 28, 2025 •

edited

Loading

bcardosolopes commented Apr 28, 2025 •

edited

Loading