[Mlir-commits] [mlir] b277382 - Remove error-prone mlir::ExecutionEngine::invoke overload.

[Sean Silva]

Author: Sean Silva
Date: 2020-05-27T13:26:03-07:00
New Revision: b2773823116157aa73ea4ac01270b22042d6bb42

URL: https://github.com/llvm/llvm-project/commit/b2773823116157aa73ea4ac01270b22042d6bb42
DIFF: https://github.com/llvm/llvm-project/commit/b2773823116157aa73ea4ac01270b22042d6bb42.diff

LOG: Remove error-prone mlir::ExecutionEngine::invoke overload.

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.

Differential Revision: https://reviews.llvm.org/D80607

Added: 
    

Modified: 
    mlir/include/mlir/ExecutionEngine/ExecutionEngine.h

Removed: 
    


################################################################################
diff  --git a/mlir/include/mlir/ExecutionEngine/ExecutionEngine.h b/mlir/include/mlir/ExecutionEngine/ExecutionEngine.h
index 914cab78dee7..d0ad8326bac8 100644
--- a/mlir/include/mlir/ExecutionEngine/ExecutionEngine.h
+++ b/mlir/include/mlir/ExecutionEngine/ExecutionEngine.h
@@ -94,16 +94,9 @@ class ExecutionEngine {
   /// 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 @@ class ExecutionEngine {
   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_