-
Notifications
You must be signed in to change notification settings - Fork 6.8k
MXNet FFI for Operator Imperative Invocation #17510
Conversation
e7d2e44
to
69b989f
Compare
d29ae7f
to
f7af173
Compare
* } | ||
* \endcode | ||
*/ | ||
using FType = std::function<void (MXNetArgs args, MXNetRetValue* rv)>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a question about the ABI compatibility of std::function.
std::function is a STL container, which has different implementations between different versions of compiler (e.g. gcc4 and gcc5) or different compilers (gcc and clang).
For example, MXNet is built in gcc4, which uses the reference & to store the function arguments. However, the other application is built in gcc5, which uses the rvalue reference && to store the function arguments. The other application could not call the MXNet API by PackedFunc, since the implementations of std::function are different between gcc4 and gcc5.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @wkcn :). Basically PackedFunc
does not go across the dll boundary. It remains in the MXNet side.
When application A calls PackedFunc, it first passes the function name to MXNet. Then MXNet finds the corresponding PackedFunc, and returns the address of the PackedFunc back to A (A may cache the name-address map). Finally, A invokes that PackedFunc with the address.
To conclude, A is not aware of std::function. It only gets an address which is a ABI-compatible void*.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hzfan Thank you!
Yes. In the application A, we can call MXNetFuncCall
to call the PackedFunc, call MXNetFuncFree
to free the PackedFunc, and call MXNetFuncCreateFromCFunc
like TVMFuncCreateFromCFunc in TVM to create new PackedFunc.
However, when the implementations of std::function
are different between MXNet and the application A, A could not call PackedFunc directly, namely
PackedFunc *func = GetAPackedFuncFromMXNet();
(*func)(a, b, c);
Besides, A could not pass its PackedFunc to MXNet directly, namely
PackedFunc func = GetAPackedFuncFromA();
MXNetFuncCall(&func, xxx);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wkcn Thanks for bringing this up. Yes, I agree that the above two cases trigger unexpected errors.
In the first case, PackedFunc is exposed to A, which is not intended. Also, in this pr, application A is basically cython. As can be seen from function.pxi
and convert.pxi
, PackedFunc is not exposed to cython. So the first case does not occur in this pr.
As for the second case, currently this FFI does not support receiving callback functions from outside of MXNet (MXNetFuncCreateFromCFunc
does not exist). So the second case does not occur either.
Generally in this pr PackedFunc is called through the intended MXNetFuncCall
, which ensures the ABI-compatibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hzfan I see. Thank you for the detailed explaination : )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this PR ready for review?
@eric-haibin-lin Yes. |
Thank @leezu for review :) |
c5f640e
to
14ca3c8
Compare
This reverts commit 3f0b049.
* Init * Add nop * Add utility function SetInOut and Invoke * Init ctypes * Dispatch for default/CSR array * Refactor, register the funcs where they are used, except for _api_internal.py, which is registered in api.py * Seperate tvm ffi api and legacy api * Replace legacy zeros with new * Fix numpy.int64 in shape * Fix sanity * Fix * Remove python2 support * Cleanup * Fix ci * Fix lint * Revert rand_shape_nd * Fix clang-tidy * Support NDArray in ctypes * Using runtime * Conversion ctor * Tensordot * Tensordot backward * Fix nop regression * Deprecate Array * Fix comments * Fix comments * Add acknowledgement to incubator-tvm * Refactor according to comments
Description
This is a follow-up pr of #17097. It aims to reduce the imperative invoke overhead with tvm ffi when cython is enabled. I implemented
np.zeros
as a demo. Currently the data types supported by the new ffi areThe details and performance comparison can be seen at https://cwiki.apache.org/confluence/display/MXNET/MXNet+FFI+for+Operator+Imperative+Invocation .
Checklist
Essentials
Please feel free to remove inapplicable items for your PR.
Changes
Comments