[PATCH] D80607: Remove error-prone mlir::ExecutionEngine::invoke overload.
Sean Silva via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue May 26 20:42:12 PDT 2020
silvas created this revision.
silvas added a reviewer: ftynse.
Herald added subscribers: llvm-commits, jurahul, Kayjukh, frgossen, grosul1, Joonsoo, stephenneuendorffer, liufengdb, lucyrfox, mgester, arpith-jacob, nicolasvasilache, antiagainst, shauheen, jpienaar, rriddle, mehdi_amini.
Herald added a project: LLVM.
I just spent a bunch of time debugging a mysterious bug that ended being due to my SmallVector getting passed to the Args&... overload instead of the MutableArrayRef overload, with disastrous results.
I appreciate the intent of this API, but for a function that does a bunch of unsafe casts, adding in potential overload confusion is just too much C++ footgun. If we end up needing this functionality, having something like a separate `packArgs(Args&...) -> SmallVector` overload would be preferable.
Turns out this API is unused and untested (even out of tree as far as I can tell, modulo the optional passing of no args to the other invoke as I fixed in this patch), so it's an easy fix -- just delete it and touch up the other overload.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D80607
Files:
mlir/include/mlir/ExecutionEngine/ExecutionEngine.h
Index: mlir/include/mlir/ExecutionEngine/ExecutionEngine.h
===================================================================
--- mlir/include/mlir/ExecutionEngine/ExecutionEngine.h
+++ mlir/include/mlir/ExecutionEngine/ExecutionEngine.h
@@ -94,16 +94,9 @@
/// pointer to it. Propagates errors in case of failure.
llvm::Expected<void (*)(void **)> lookup(StringRef name) const;
- /// Invokes the function with the given name passing it the list of arguments.
- /// The arguments are accepted by lvalue-reference since the packed function
- /// interface expects a list of non-null pointers.
- template <typename... Args>
- llvm::Error invoke(StringRef name, Args &... args);
-
/// Invokes the function with the given name passing it the list of arguments
- /// as a list of opaque pointers. This is the arity-agnostic equivalent of
- /// the templated `invoke`.
- llvm::Error invoke(StringRef name, MutableArrayRef<void *> args);
+ /// as a list of opaque pointers.
+ llvm::Error invoke(StringRef name, MutableArrayRef<void *> args = llvm::None);
/// Set the target triple on the module. This is implicitly done when creating
/// the engine.
@@ -135,19 +128,6 @@
llvm::JITEventListener *perfListener;
};
-template <typename... Args>
-llvm::Error ExecutionEngine::invoke(StringRef name, Args &... args) {
- auto expectedFPtr = lookup(name);
- if (!expectedFPtr)
- return expectedFPtr.takeError();
- auto fptr = *expectedFPtr;
-
- SmallVector<void *, 8> packedArgs{static_cast<void *>(&args)...};
- (*fptr)(packedArgs.data());
-
- return llvm::Error::success();
-}
-
} // end namespace mlir
#endif // MLIR_EXECUTIONENGINE_EXECUTIONENGINE_H_
-------------- next part --------------
A non-text attachment was scrubbed...
Name: D80607.266406.patch
Type: text/x-patch
Size: 1689 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200527/92b61568/attachment.bin>
More information about the llvm-commits
mailing list