You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2021/01/05 04:48:11 UTC
[GitHub] [tvm] masahi opened a new pull request #7210: [VM] Per-input, data dependence speficiation for shape func
masahi opened a new pull request #7210:
URL: https://github.com/apache/tvm/pull/7210
Thanks for contributing to TVM! Please refer to guideline https://tvm.apache.org/docs/contribute/ for useful information and tips. After the pull request is submitted, please request code reviews from [Reviewers](https://github.com/apache/incubator-tvm/blob/master/CONTRIBUTORS.md#reviewers) by @ them in the pull request thread.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi commented on a change in pull request #7210: [VM] Per-input, data dependence specification for shape func
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #7210:
URL: https://github.com/apache/tvm/pull/7210#discussion_r557764451
##########
File path: include/tvm/relay/op_attr_types.h
##########
@@ -85,7 +85,7 @@ using TNonComputational = bool;
/*!
* \brief Mark the operator whether output shape is data dependant.
*/
-using TShapeDataDependant = bool;
+using TShapeDataDependant = Array<Integer>;
Review comment:
I did try `Array<Bool>` at first, but it didn't compile
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] icemelon9 commented on a change in pull request #7210: [VM] Per-input, data dependence specification for shape func
Posted by GitBox <gi...@apache.org>.
icemelon9 commented on a change in pull request #7210:
URL: https://github.com/apache/tvm/pull/7210#discussion_r557782915
##########
File path: src/relay/backend/compile_engine.cc
##########
@@ -593,8 +604,10 @@ class MakeShapeFunc : public backend::MemoizedExprTranslator<Array<te::Tensor>>
std::unordered_map<Expr, Array<te::Tensor>, ObjectPtrHash, ObjectPtrEqual> param_data_;
/*! \brief Map from parameter to list of shape placeholder */
std::unordered_map<Expr, Array<te::Tensor>, ObjectPtrHash, ObjectPtrEqual> param_shapes_;
- /*! \brief Stack of data dependencies for shape function */
+ /*! \brief Stack of data dependencies for shape function, specified per op */
std::vector<bool> data_dependants_;
Review comment:
btw. It's a typo for `dependant`. It should be `dependent`. Could you correct it at the same time?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi commented on pull request #7210: [VM] Per-input, data dependence specification for shape func
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #7210:
URL: https://github.com/apache/tvm/pull/7210#issuecomment-759845518
cc @icemelon9 @zhiics Any thought on this issue? I think this is important to discuss.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] mbrookhart commented on pull request #7210: [VM] Per-input, data dependence specification for shape func
Posted by GitBox <gi...@apache.org>.
mbrookhart commented on pull request #7210:
URL: https://github.com/apache/tvm/pull/7210#issuecomment-754802181
I like this, I think it makes a lot of sense, but I'll defer mainly to @zhiics and @icemelon9 since they mainly implemented the infrastructure for heterogeneous shape functions.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi commented on a change in pull request #7210: [VM] Per-input, data dependence specification for shape func
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #7210:
URL: https://github.com/apache/tvm/pull/7210#discussion_r557784553
##########
File path: src/relay/backend/compile_engine.cc
##########
@@ -593,8 +604,10 @@ class MakeShapeFunc : public backend::MemoizedExprTranslator<Array<te::Tensor>>
std::unordered_map<Expr, Array<te::Tensor>, ObjectPtrHash, ObjectPtrEqual> param_data_;
/*! \brief Map from parameter to list of shape placeholder */
std::unordered_map<Expr, Array<te::Tensor>, ObjectPtrHash, ObjectPtrEqual> param_shapes_;
- /*! \brief Stack of data dependencies for shape function */
+ /*! \brief Stack of data dependencies for shape function, specified per op */
std::vector<bool> data_dependants_;
Review comment:
ok I'll try removing `data_dependants_`.
Right, I was not sure `dependant` was intentional or not... It's actually everywhere (grep `dependant` gives many hit). I'll fix them all!!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi commented on pull request #7210: [VM] Per-input, data dependence specification for shape func
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #7210:
URL: https://github.com/apache/tvm/pull/7210#issuecomment-761173735
@icemelon9 it should be ready
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi commented on a change in pull request #7210: [VM] Per-input, data dependence specification for shape func
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #7210:
URL: https://github.com/apache/tvm/pull/7210#discussion_r557780399
##########
File path: src/relay/backend/compile_engine.cc
##########
@@ -593,8 +604,10 @@ class MakeShapeFunc : public backend::MemoizedExprTranslator<Array<te::Tensor>>
std::unordered_map<Expr, Array<te::Tensor>, ObjectPtrHash, ObjectPtrEqual> param_data_;
/*! \brief Map from parameter to list of shape placeholder */
std::unordered_map<Expr, Array<te::Tensor>, ObjectPtrHash, ObjectPtrEqual> param_shapes_;
- /*! \brief Stack of data dependencies for shape function */
+ /*! \brief Stack of data dependencies for shape function, specified per op */
std::vector<bool> data_dependants_;
Review comment:
There is a usage of `data_dependants_` at https://github.com/apache/tvm/blob/f658556d5f9491138fde4451baf60c9621d0755d/src/relay/backend/compile_engine.cc#L453
I didn't look into this in detail, can we replace this with `data_dependants_per_input_`? To me it seems per-op data dependant makes more sense here. Or `data_dependants_per_input_.back()` would work here?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] icemelon9 merged pull request #7210: [VM] Per-input, data dependence specification for shape func
Posted by GitBox <gi...@apache.org>.
icemelon9 merged pull request #7210:
URL: https://github.com/apache/tvm/pull/7210
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] icemelon9 commented on a change in pull request #7210: [VM] Per-input, data dependence specification for shape func
Posted by GitBox <gi...@apache.org>.
icemelon9 commented on a change in pull request #7210:
URL: https://github.com/apache/tvm/pull/7210#discussion_r557777536
##########
File path: include/tvm/relay/op_attr_types.h
##########
@@ -85,7 +85,7 @@ using TNonComputational = bool;
/*!
* \brief Mark the operator whether output shape is data dependant.
*/
-using TShapeDataDependant = bool;
+using TShapeDataDependant = Array<Integer>;
Review comment:
I see. We can use integer then.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi commented on a change in pull request #7210: [VM] Per-input, data dependence specification for shape func
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #7210:
URL: https://github.com/apache/tvm/pull/7210#discussion_r557780399
##########
File path: src/relay/backend/compile_engine.cc
##########
@@ -593,8 +604,10 @@ class MakeShapeFunc : public backend::MemoizedExprTranslator<Array<te::Tensor>>
std::unordered_map<Expr, Array<te::Tensor>, ObjectPtrHash, ObjectPtrEqual> param_data_;
/*! \brief Map from parameter to list of shape placeholder */
std::unordered_map<Expr, Array<te::Tensor>, ObjectPtrHash, ObjectPtrEqual> param_shapes_;
- /*! \brief Stack of data dependencies for shape function */
+ /*! \brief Stack of data dependencies for shape function, specified per op */
std::vector<bool> data_dependants_;
Review comment:
There is a usage of `data_dependants_` at https://github.com/apache/tvm/blob/f658556d5f9491138fde4451baf60c9621d0755d/src/relay/backend/compile_engine.cc#L453
I didn't look into this in detail, can we replace this with `data_dependants_per_input_`? To me it seems per-op data dependant makes more sense here.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] icemelon9 commented on a change in pull request #7210: [VM] Per-input, data dependence specification for shape func
Posted by GitBox <gi...@apache.org>.
icemelon9 commented on a change in pull request #7210:
URL: https://github.com/apache/tvm/pull/7210#discussion_r557753221
##########
File path: include/tvm/relay/op_attr_types.h
##########
@@ -85,7 +85,7 @@ using TNonComputational = bool;
/*!
* \brief Mark the operator whether output shape is data dependant.
*/
-using TShapeDataDependant = bool;
+using TShapeDataDependant = Array<Integer>;
Review comment:
Maybe using `Bool` is better here?
##########
File path: src/relay/backend/compile_engine.cc
##########
@@ -518,16 +518,27 @@ class MakeShapeFunc : public backend::MemoizedExprTranslator<Array<te::Tensor>>
<< "Internal error, cannot find TShapeDataDependant for " << op->name;
data_dependants_.push_back(IsDataDependant(call_node));
+
+ Array<Integer> dep_spec = tshape_data_dependant[op];
+ if (dep_spec.size() == 1 && call_node->args.size() > 1) {
Review comment:
```suggestion
if (dep_spec.size() == 1) {
```
##########
File path: src/relay/backend/compile_engine.cc
##########
@@ -593,8 +604,10 @@ class MakeShapeFunc : public backend::MemoizedExprTranslator<Array<te::Tensor>>
std::unordered_map<Expr, Array<te::Tensor>, ObjectPtrHash, ObjectPtrEqual> param_data_;
/*! \brief Map from parameter to list of shape placeholder */
std::unordered_map<Expr, Array<te::Tensor>, ObjectPtrHash, ObjectPtrEqual> param_shapes_;
- /*! \brief Stack of data dependencies for shape function */
+ /*! \brief Stack of data dependencies for shape function, specified per op */
std::vector<bool> data_dependants_;
Review comment:
I think you should remove the `data_dependants_` and only use `data_dependants_per_input_`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] icemelon9 commented on pull request #7210: [VM] Per-input, data dependence specification for shape func
Posted by GitBox <gi...@apache.org>.
icemelon9 commented on pull request #7210:
URL: https://github.com/apache/tvm/pull/7210#issuecomment-761204133
Thanks @masahi @zhiics @mbrookhart
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] zhiics commented on a change in pull request #7210: [VM] Per-input, data dependence specification for shape func
Posted by GitBox <gi...@apache.org>.
zhiics commented on a change in pull request #7210:
URL: https://github.com/apache/tvm/pull/7210#discussion_r556975451
##########
File path: python/tvm/relay/op/op.py
##########
@@ -364,7 +364,7 @@ def register_shape_func(op_name, data_dependant, shape_func=None, level=10):
op_name : str
The name of the op.
- data_dependant : bool
+ data_dependant : bool or list of bool
Review comment:
Let's have better doc for this so that users know that it could be a list of booleans that indicates if each of the input is data dependent of not.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] icemelon9 commented on a change in pull request #7210: [VM] Per-input, data dependence specification for shape func
Posted by GitBox <gi...@apache.org>.
icemelon9 commented on a change in pull request #7210:
URL: https://github.com/apache/tvm/pull/7210#discussion_r557782672
##########
File path: src/relay/backend/compile_engine.cc
##########
@@ -593,8 +604,10 @@ class MakeShapeFunc : public backend::MemoizedExprTranslator<Array<te::Tensor>>
std::unordered_map<Expr, Array<te::Tensor>, ObjectPtrHash, ObjectPtrEqual> param_data_;
/*! \brief Map from parameter to list of shape placeholder */
std::unordered_map<Expr, Array<te::Tensor>, ObjectPtrHash, ObjectPtrEqual> param_shapes_;
- /*! \brief Stack of data dependencies for shape function */
+ /*! \brief Stack of data dependencies for shape function, specified per op */
std::vector<bool> data_dependants_;
Review comment:
Correct me if I'm wrong. The `data_dependent_` logic is to determine whether we need shape or data from each node when visiting it. This applies to `Var`, `Constant`, and `Call`. Since the data dependency could be now different for each arg in the Call, we should always use `data_dependants_per_input_`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org