gemini-code-assist[bot] commented on code in PR #100:
URL: https://github.com/apache/tvm-ffi/pull/100#discussion_r2422858601
##########
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[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:
This function is a bit hard to read with all the nested `reinterpret_cast`s.
Consider using intermediate variables to improve readability.
```c
void _mlir_add_one_c_symbol(void** packed_args) {
void* handle = *reinterpret_cast<void**>(packed_args[0]);
const TVMFFIAny* args = *reinterpret_cast<const
TVMFFIAny**>(packed_args[1]);
int32_t num_args = *reinterpret_cast<int32_t*>(packed_args[2]);
TVMFFIAny* rv = *reinterpret_cast<TVMFFIAny**>(packed_args[3]);
int* ret_code = reinterpret_cast<int*>(packed_args[4]);
*ret_code = __add_one_c_symbol(handle, args, num_args, rv);
}
```
##########
python/tvm_ffi/cython/tvm_ffi_python_helpers.h:
##########
@@ -592,6 +592,91 @@ 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.
+ *
+ * The MLIR execution engine generates functions that correspond 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, the MLIR execution engine should be able to support
+ * some form of "extern C" feature that directly exposes the function pointers
+ * of C-compatible functions with an attribute tag. So we keep this feature
+ * in the Python helper layer for now in case the 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 of following the redirection pattern
+ * in `TVMFFIPyMLIRPackedSafeCall::Invoke`.
+ *
+ * \sa TVMFFIPyMLIRPackedSafeCall::Invoke
+ */
+class TVMFFIPyMLIRPackedSafeCall {
+ public:
+ TVMFFIPyMLIRPackedSafeCall(void (*mlir_packed_safe_call)(void**),
+ PyObject* keep_alive_object) noexcept
+ : 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 TVMFFIPyMLIRPackedSafeCall handle
+ * \param mlir_packed_safe_call The MLIR packed safe call function
+ * \param keep_alive_object The keep alive object
+ * \return The TVMFFIPyMLIRPackedSafeCall object
+ */
+void* TVMFFIPyMLIRPackedSafeCallCreate(void (*mlir_packed_safe_call)(void**),
+ PyObject* keep_alive_object) {
Review Comment:
The `noexcept` specifier is missing from the `mlir_packed_safe_call`
function pointer type in `TVMFFIPyMLIRPackedSafeCallCreate`'s signature, which
is inconsistent with its declaration in `python/tvm_ffi/cython/base.pxi`.
```c
void* TVMFFIPyMLIRPackedSafeCallCreate(void (*mlir_packed_safe_call)(void**)
noexcept,
PyObject* keep_alive_object) {
```
##########
python/tvm_ffi/cython/tvm_ffi_python_helpers.h:
##########
@@ -592,6 +592,91 @@ 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.
+ *
+ * The MLIR execution engine generates functions that correspond 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, the MLIR execution engine should be able to support
+ * some form of "extern C" feature that directly exposes the function pointers
+ * of C-compatible functions with an attribute tag. So we keep this feature
+ * in the Python helper layer for now in case the 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 of following the redirection pattern
+ * in `TVMFFIPyMLIRPackedSafeCall::Invoke`.
+ *
+ * \sa TVMFFIPyMLIRPackedSafeCall::Invoke
+ */
+class TVMFFIPyMLIRPackedSafeCall {
+ public:
+ TVMFFIPyMLIRPackedSafeCall(void (*mlir_packed_safe_call)(void**),
+ PyObject* keep_alive_object) noexcept
+ : 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_;
Review Comment:
The `noexcept` specifier is missing from the `mlir_packed_safe_call`
function pointer type in the `TVMFFIPyMLIRPackedSafeCall` class definition.
This is inconsistent with its declaration in `python/tvm_ffi/cython/base.pxi`
and the cast in `python/tvm_ffi/cython/function.pxi`. This should be added for
type consistency and to prevent potential compilation issues.
```c
TVMFFIPyMLIRPackedSafeCall(void (*mlir_packed_safe_call)(void**) noexcept,
PyObject* keep_alive_object) noexcept
: 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**) noexcept;
PyObject* keep_alive_object_;
```
--
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]
