[MLIR] Fix mlirExecutionEngineLookup throwing assert on lookup fail #123924
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-mlir @llvm/pr-subscribers-mlir-execution-engine Author: Edgar (edg-l) ChangesApparently trying to lookup a function pointer using the C api The docs itself says it returns a nullptr when no function is found so it should be sensible to not throw an assert in this case. Full diff: https://github.com/llvm/llvm-project/pull/123924.diff 1 Files Affected:
diff --git a/mlir/lib/CAPI/ExecutionEngine/ExecutionEngine.cpp b/mlir/lib/CAPI/ExecutionEngine/ExecutionEngine.cpp
index 507be9171d328d..8480a5409a8159 100644
--- a/mlir/lib/CAPI/ExecutionEngine/ExecutionEngine.cpp
+++ b/mlir/lib/CAPI/ExecutionEngine/ExecutionEngine.cpp
@@ -15,6 +15,7 @@
#include "mlir/Target/LLVMIR/Dialect/LLVMIR/LLVMToLLVMIRTranslation.h"
#include "mlir/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.h"
#include "llvm/ExecutionEngine/Orc/Mangling.h"
+#include "llvm/Support/Error.h"
#include "llvm/Support/TargetSelect.h"
using namespace mlir;
@@ -85,18 +86,20 @@ mlirExecutionEngineInvokePacked(MlirExecutionEngine jit, MlirStringRef name,
extern "C" void *mlirExecutionEngineLookupPacked(MlirExecutionEngine jit,
MlirStringRef name) {
- auto expectedFPtr = unwrap(jit)->lookupPacked(unwrap(name));
- if (!expectedFPtr)
+ auto optionalFPtr =
+ llvm::expectedToOptional(unwrap(jit)->lookup(unwrap(name)));
+ if (!optionalFPtr.has_value())
return nullptr;
- return reinterpret_cast<void *>(*expectedFPtr);
+ return reinterpret_cast<void *>(*optionalFPtr);
}
extern "C" void *mlirExecutionEngineLookup(MlirExecutionEngine jit,
MlirStringRef name) {
- auto expectedFPtr = unwrap(jit)->lookup(unwrap(name));
- if (!expectedFPtr)
+ auto optionalFPtr =
+ llvm::expectedToOptional(unwrap(jit)->lookup(unwrap(name)));
+ if (!optionalFPtr.has_value())
return nullptr;
- return reinterpret_cast<void *>(*expectedFPtr);
+ return reinterpret_cast<void *>(*optionalFPtr);
}
extern "C" void mlirExecutionEngineRegisterSymbol(MlirExecutionEngine jit,
|
edg-l
force-pushed
the
mlir_c_exec_engine_lookup
branch
from
06f7955 to
7198fb2
Compare
Lewuathe
reviewed
Jan 24, 2025
There was a problem hiding this comment.
It appears that a couple of tests of execution engine fails. This Python code is likely to use the CAPI as well. Could you check whether it's related to this change or not?
https://buildkite.com/llvm-project/github-pull-requests/builds/139352
# RUN: at line 1
env MLIR_RUNNER_UTILS=/var/lib/buildkite-agent/builds/linux-56-59b8f5d88-gz48w-1/llvm-project/github-pull-requests/build/lib/libmlir_runner_utils.so MLIR_C_RUNNER_UTILS=/var/lib/buildkite-agent/builds/linux-56-59b8f5d88-gz48w-1/llvm-project/github-pull-requests/build/lib/libmlir_c_runner_utils.so /usr/bin/python3.10 /var/lib/buildkite-agent/builds/linux-56-59b8f5d88-gz48w-1/llvm-project/github-pull-requests/mlir/test/python/execution_engine.py 2>&1 | /var/lib/buildkite-agent/builds/linux-56-59b8f5d88-gz48w-1/llvm-project/github-pull-requests/build/bin/FileCheck /var/lib/buildkite-agent/builds/linux-56-59b8f5d88-gz48w-1/llvm-project/github-pull-requests/mlir/test/python/execution_engine.py
# executed command: env MLIR_RUNNER_UTILS=/var/lib/buildkite-agent/builds/linux-56-59b8f5d88-gz48w-1/llvm-project/github-pull-requests/build/lib/libmlir_runner_utils.so MLIR_C_RUNNER_UTILS=/var/lib/buildkite-agent/builds/linux-56-59b8f5d88-gz48w-1/llvm-project/github-pull-requests/build/lib/libmlir_c_runner_utils.so /usr/bin/python3.10 /var/lib/buildkite-agent/builds/linux-56-59b8f5d88-gz48w-1/llvm-project/github-pull-requests/mlir/test/python/execution_engine.py
# note: command had no output on stdout or stderr
# error: command failed with exit status: -11
# executed command: /var/lib/buildkite-agent/builds/linux-56-59b8f5d88-gz48w-1/llvm-project/github-pull-requests/build/bin/FileCheck /var/lib/buildkite-agent/builds/linux-56-59b8f5d88-gz48w-1/llvm-project/github-pull-requests/mlir/test/python/execution_engine.py
# .---command stderr------------
# | /var/lib/buildkite-agent/builds/linux-56-59b8f5d88-gz48w-1/llvm-project/github-pull-requests/mlir/test/python/execution_engine.py:129:11: error: CHECK: expected string not found in input
# | # CHECK: 42.0 + 2.0 = 44.0
# | ^
# | <stdin>:12:25: note: scanning from here
# | TEST: testInvokeFloatAdd
Nvm, looks like invoke calls lookup so maybe its related |
|
I found the issue, copy pasted the lookup into lookuppacked and forgot to change lookup to lookuppacked. @Lewuathe should be fixed now |
ftynse
approved these changes
Jan 26, 2025
Lewuathe
approved these changes
Jan 28, 2025
|
I guess its ready to be merged? |
edg-l
force-pushed
the
mlir_c_exec_engine_lookup
branch
from
0a52b59 to
9b579b3
Compare
edg-l
force-pushed
the
mlir_c_exec_engine_lookup
branch
from
9b579b3 to
abd362d
Compare
|
I rebased and all, this has been ready for quite a while. Can you look into merging this? |
sivan-shani
pushed a commit
to sivan-shani/llvm-project
that referenced
this pull request
Feb 24, 2025
…lvm#123924) Apparently trying to lookup a function pointer using the C api `mlirExecutionEngineLookup` will throw an assert instead of just returning a nullptr on builds with asserts. The docs itself says it returns a nullptr when no function is found so it should be sensible to not throw an assert in this case.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Apparently trying to lookup a function pointer using the C api
mlirExecutionEngineLookupwill throw an assert instead of just returning a nullptr on builds with asserts.The docs itself says it returns a nullptr when no function is found so it should be sensible to not throw an assert in this case.