gemini-code-assist[bot] commented on code in PR #100:
URL: https://github.com/apache/tvm-ffi/pull/100#discussion_r2422855312
##########
src/ffi/extra/testing.cc:
##########
@@ -180,6 +180,13 @@ int __add_one_c_symbol(void*, const TVMFFIAny* args,
int32_t num_args, TVMFFIAny
TVM_FFI_SAFE_CALL_END();
}
+void _mlir_add_one_c_symbol(void** packed_args) {
+ reinterpret_cast<int*>(packed_args[0])[4] = __add_one_c_symbol(
+ *reinterpret_cast<void**>(packed_args[0]),
+ *reinterpret_cast<const TVMFFIAny**>(packed_args[1]),
+ *reinterpret_cast<int32_t*>(packed_args[2]),
*reinterpret_cast<TVMFFIAny**>(packed_args[3]));
+}
Review Comment:
There appears to be a bug in how the return code is being handled in this
simulated MLIR packed function. The expression
`reinterpret_cast<int*>(packed_args[0])[4]` attempts to write to an
out-of-bounds memory location, which is undefined behavior. `packed_args[0]`
points to the `handle` argument, and accessing an offset of `[4]` from it is
incorrect. The return code should be written to the location pointed to by
`packed_args[4]`. The test might be passing due to stack layout coincidence,
but this should be fixed to avoid flaky tests and ensure correctness.
```c
void _mlir_add_one_c_symbol(void** packed_args) {
*reinterpret_cast<int*>(packed_args[4]) = __add_one_c_symbol(
*reinterpret_cast<void**>(packed_args[0]),
*reinterpret_cast<const TVMFFIAny**>(packed_args[1]),
*reinterpret_cast<int32_t*>(packed_args[2]),
*reinterpret_cast<TVMFFIAny**>(packed_args[3]));
}
```
##########
python/tvm_ffi/cython/tvm_ffi_python_helpers.h:
##########
@@ -592,6 +592,90 @@ TVM_FFI_INLINE void
TVMFFIPyPushTempPyObject(TVMFFIPyCallContext* ctx, PyObject*
ctx->temp_py_objects[ctx->num_temp_py_objects++] = arg;
}
+//----------------------------------------------------------
+// Helpers for MLIR redirection
+//----------------------------------------------------------
+/*!
+ * \brief Function specialization that leverages MLIR packed safe call
definitions.
+ *
+ * MLIR Execution engine generates function that corresponds to the packed
signature.
+ * As of now it is hard to access the raw extern C function pointer of SafeCall
+ * directly when we declare the signature in LLVM dialect.
+ *
+ * Note that in theory MLIR execution engine should be able to also support
+ * some form of "extern C" feature that directly exposes the funtion pointers
+ * of C-compatible functions with an attribute tag. So we keep this feature
+ * in the python helper layer for now in case MLIR execution engine supports
it in the future.
+ *
+ * This helper enables us to create ffi::Function from the MLIR packed
+ * safe call function pointer instead following the redirection pattern
+ * in `TVMFFIPyMLIRPackedSafeCall::Invoke`.
+ *
+ * \sa TVMFFIPyMLIRPackedSafeCall::Invoke
+ */
+class TVMFFIPyMLIRPackedSafeCall {
+ public:
+ TVMFFIPyMLIRPackedSafeCall(void (*mlir_packed_safe_call)(void**), PyObject*
keep_alive_object)
Review Comment:
The Cython code casts the function pointer to a `noexcept` function pointer,
but the C++ definitions here do not include the `noexcept` specifier. For
consistency and to correctly reflect the contract of the function, please add
`noexcept` to the function pointer types in this file:
- `TVMFFIPyMLIRPackedSafeCall` constructor (this line)
- `mlir_packed_safe_call_` member variable (line 643)
- `TVMFFIPyMLIRPackedSafeCallCreate` function (line 653)
##########
python/tvm_ffi/cython/function.pxi:
##########
@@ -690,8 +693,9 @@ cdef class Function(Object):
which is the function handle
keep_alive_object : object
- optional closure to be captured and kept alive
+ optional object to be captured and kept alive
Usually can be the execution engine that JITed the function
+ to ensure we keep th
Review Comment:
This line in the docstring appears to be incomplete. It was likely intended
to be similar to the docstring in the new `__from_mlir_packed_safe_call__`
function.
```
to ensure we keep the execution environment alive.
```
##########
python/tvm_ffi/cython/tvm_ffi_python_helpers.h:
##########
@@ -592,6 +592,90 @@ TVM_FFI_INLINE void
TVMFFIPyPushTempPyObject(TVMFFIPyCallContext* ctx, PyObject*
ctx->temp_py_objects[ctx->num_temp_py_objects++] = arg;
}
+//----------------------------------------------------------
+// Helpers for MLIR redirection
+//----------------------------------------------------------
+/*!
+ * \brief Function specialization that leverages MLIR packed safe call
definitions.
+ *
+ * MLIR Execution engine generates function that corresponds to the packed
signature.
+ * As of now it is hard to access the raw extern C function pointer of SafeCall
+ * directly when we declare the signature in LLVM dialect.
+ *
+ * Note that in theory MLIR execution engine should be able to also support
+ * some form of "extern C" feature that directly exposes the funtion pointers
+ * of C-compatible functions with an attribute tag. So we keep this feature
+ * in the python helper layer for now in case MLIR execution engine supports
it in the future.
+ *
+ * This helper enables us to create ffi::Function from the MLIR packed
+ * safe call function pointer instead following the redirection pattern
+ * in `TVMFFIPyMLIRPackedSafeCall::Invoke`.
+ *
+ * \sa TVMFFIPyMLIRPackedSafeCall::Invoke
+ */
+class TVMFFIPyMLIRPackedSafeCall {
+ public:
+ TVMFFIPyMLIRPackedSafeCall(void (*mlir_packed_safe_call)(void**), PyObject*
keep_alive_object)
+ : mlir_packed_safe_call_(mlir_packed_safe_call),
keep_alive_object_(keep_alive_object) {
+ if (keep_alive_object_) {
+ Py_IncRef(keep_alive_object_);
+ }
+ }
+
+ ~TVMFFIPyMLIRPackedSafeCall() {
+ if (keep_alive_object_) {
+ Py_DecRef(keep_alive_object_);
+ }
+ }
+
+ static int Invoke(void* func, const TVMFFIAny* args, int32_t num_args,
TVMFFIAny* rv) {
+ TVMFFIPyMLIRPackedSafeCall* self =
reinterpret_cast<TVMFFIPyMLIRPackedSafeCall*>(func);
+ int ret_code = 0;
+ void* handle = nullptr;
+ void* mlir_args[] = {&handle, const_cast<TVMFFIAny**>(&args), &num_args,
&rv, &ret_code};
+ (*self->mlir_packed_safe_call_)(mlir_args);
+ return ret_code;
+ }
+
+ static void Deleter(void* self) { delete
static_cast<TVMFFIPyMLIRPackedSafeCall*>(self); }
+
+ private:
+ void (*mlir_packed_safe_call_)(void**);
+ PyObject* keep_alive_object_;
+};
+
+/*!
+ * \brief Create a TVMFFIPyMLIRPackedExternC handle
+ * \param mlir_packed_safe_call The MLIR packed safe call function
+ * \param keep_alive_object The keep alive object
+ * \return The TVMFFIPyMLIRPackedExternC object
+ */
+void* TVMFFIPyMLIRPackedSafeCallCreate(void (*mlir_packed_safe_call)(void**),
+ PyObject* keep_alive_object) {
+ return new TVMFFIPyMLIRPackedSafeCall(mlir_packed_safe_call,
keep_alive_object);
+}
+
+/*!
+ * \brief Call the MLIR packed safe call function
+ * \param self The TVMFFIPyMLIRPackedExternC object
+ * \param args The arguments
+ * \param num_args The number of arguments
+ * \param rv The result
+ * \return The return code
+ */
+int TVMFFIPyMLIRPackedSafeCallInvoke(void* self, const TVMFFIAny* args,
int32_t num_args,
+ TVMFFIAny* rv) {
+ return TVMFFIPyMLIRPackedSafeCall::Invoke(self, args, num_args, rv);
+}
+
+/*!
+ * \brief Delete the TVMFFIPyMLIRPackedExternC object
+ * \param self The TVMFFIPyMLIRPackedExternC object
+ */
+void TVMFFIPyMLIRPackedSafeCallDeleter(void* self) {
+ return TVMFFIPyMLIRPackedSafeCall::Deleter(self);
+}
Review Comment:
The Doxygen comments for `TVMFFIPyMLIRPackedSafeCallCreate`,
`TVMFFIPyMLIRPackedSafeCallInvoke`, and `TVMFFIPyMLIRPackedSafeCallDeleter`
incorrectly refer to `TVMFFIPyMLIRPackedExternC`. This appears to be a
copy-paste error and should be corrected to `TVMFFIPyMLIRPackedSafeCall`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
