Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

Infra to use tvm write op kernels #15550

Merged
merged 9 commits into from
Jul 22, 2019
Merged

Infra to use tvm write op kernels #15550

merged 9 commits into from
Jul 22, 2019

Conversation

yzhliu
Copy link
Member

@yzhliu yzhliu commented Jul 16, 2019

Description

This PR implements an infra to let users write op kernels in pure Python. One broadcast operator example can be found in,

  • Kernel definition: contrib/tvmop/basic/ufunc.py
  • Operator registry: src/operator/contrib/tvmop/ufunc.cc
  • Test case: tests/python/unittest/test_tvm_op.py

More background and discussion can be found here: #15465

void TVMBroadcastCompute(const nnvm::NodeAttrs& attrs,
const mxnet::OpContext& ctx,
const std::vector<TBlob>& inputs,
const std::vector<OpReqType>& req,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why req is ignored?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just an example to show how it works

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean does tvm have good support for all kinds of OpReqType, like inplace, addto?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good question. I don't have a good answer to it as for now.

Makefile Show resolved Hide resolved
@@ -734,3 +734,8 @@ def write_all_str(module_file, module_all_list):

ctypes.pythonapi.PyCapsule_New.restype = ctypes.py_object
ctypes.pythonapi.PyCapsule_GetPointer.restype = ctypes.c_void_p

from .runtime import Features
if Features().is_enabled("TVM_OP"):
Copy link
Contributor

@ZhennanQin ZhennanQin Jul 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems TVM only supports python because of this code. Shall we load libtvmop automatically along with loading libmxnet? Then we don't do extra change with front-end language.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be doable. while might need non-trivial surgery to both. cc @tqchen if you have insight.

Makefile Show resolved Hide resolved
Copy link
Contributor

@marcoabreu marcoabreu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry Yizhi, but I'm still waiting for cmake support. We are making more and more changes in make.

Thus, before we proceed, please add cmake.

@yzhliu yzhliu requested a review from rahul003 as a code owner July 17, 2019 02:37
@yzhliu
Copy link
Member Author

yzhliu commented Jul 17, 2019

@marcoabreu cmake support added

@marcoabreu marcoabreu self-requested a review July 17, 2019 06:33
CMakeLists.txt Show resolved Hide resolved
*module_ptr_ = module;
}

inline PackedFunc GetFunction(const std::shared_ptr<Module> &module,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please no inline.

@yzhliu
Copy link
Member Author

yzhliu commented Jul 18, 2019

Please review again @tqchen @marcoabreu @ZhennanQin @larroy

Copy link
Contributor

@marcoabreu marcoabreu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved regarding build-system

----------
name : str
function name
target : bool
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bool ->str

@@ -185,6 +189,9 @@ enum : unsigned {
SIGNAL_HANDLER,
DEBUG,

// TVM operator
TVM_OP,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great!

Copy link
Contributor

@larroy larroy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM % minor comments

Copy link
Contributor

@ZhennanQin ZhennanQin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM as the first integration PR. Also need incremental PRs to address below issues:

  • Support OpReqType
  • Support Windows platform
  • Support all front-end languages
  • Propagate MXNet feature to libtvmop

@karan6181
Copy link
Contributor

@mxnet-label-bot add [Backend, Operator, pr-awaiting-response]

@marcoabreu marcoabreu added Backend Issues related to the backend of MXNet Operator pr-awaiting-response PR is reviewed and waiting for contributor to respond labels Jul 19, 2019
contrib/tvmop/prepare_tvm.sh Outdated Show resolved Hide resolved
Copy link
Member

@junrushao junrushao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@szha szha merged commit 77254f2 into apache:master Jul 22, 2019
Copy link
Contributor

@mikemwx mikemwx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

.set_attr<mxnet::FCompute>("FCompute<cpu>", mxnet::op::TVMBroadcastCompute<func_vadd_cpu>)
#if MXNET_USE_CUDA
.set_attr<mxnet::FCompute>("FCompute<gpu>", mxnet::op::TVMBroadcastCompute<func_vadd_gpu>);
#endif // MXNET_USE_CUDA
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There will be a missing semicolon if MXNET_USE_CUDA is off.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

may change to this

#if MXNET_USE_CUDA
    .set_attr<mxnet::FCompute>("FCompute<gpu>", mxnet::op::TVMBroadcastCompute<func_vadd_gpu>)
#endif  // MXNET_USE_CUDA
    .set_attr<mxnet::FCompute>("FCompute<cpu>", mxnet::op::TVMBroadcastCompute<func_vadd_cpu>);

anirudhacharya pushed a commit to anirudhacharya/mxnet that referenced this pull request Aug 20, 2019
* intra to use tvm write op kernels

* add cmake support for tvm op

* fix header lint

* cleanup USE_TVM_OP logic in Makefile

* add doc, cmake def, etc.

* fix doc

* test rand shape

* add with_seed to test

* improve err msg. add #if
access2rohit pushed a commit to access2rohit/incubator-mxnet that referenced this pull request Sep 25, 2019
* intra to use tvm write op kernels

* add cmake support for tvm op

* fix header lint

* cleanup USE_TVM_OP logic in Makefile

* add doc, cmake def, etc.

* fix doc

* test rand shape

* add with_seed to test

* improve err msg. add #if
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Backend Issues related to the backend of MXNet Operator pr-awaiting-response PR is reviewed and waiting for contributor to respond
Projects
None yet
Development

Successfully merging this pull request may close these issues.