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 2020/07/09 09:37:57 UTC
[GitHub] [incubator-tvm] lixiaoquan opened a new pull request #6024: [Relay][TF] Make StridedSlice support dynamic input and constant attrs
lixiaoquan opened a new pull request #6024:
URL: https://github.com/apache/incubator-tvm/pull/6024
@kevinthesun @mbrookhart @icemelon9 Could you please review this, thanks
The input of strided_slice is Expr and attrs are Const, but it is still possible this strided_slice is dynamic.
This PR adds / changes some test to demostrate this issue. If we make strided_slice static and add _dyn.strided_slice, the static version can't support this case, so we have to always use _dyn.strided_slice unless we are sure shape of input is static. I think other _dyn.* ops may have similiar issue
----------------------------------------------------------------
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] [incubator-tvm] kevinthesun commented on a change in pull request #6024: [Relay][TF] Make StridedSlice support dynamic input and constant attrs
Posted by GitBox <gi...@apache.org>.
kevinthesun commented on a change in pull request #6024:
URL: https://github.com/apache/incubator-tvm/pull/6024#discussion_r468093208
##########
File path: python/tvm/relay/frontend/tensorflow.py
##########
@@ -1458,6 +1458,15 @@ def _impl(inputs, attr, params, mod):
return ret
+ def _dyn():
+ for d in data_shape:
+ if not isinstance(d, int):
+ return True
+ return False
+
+ if _dyn():
Review comment:
Why do we need to special hand this in tf frontend and skip mask transformation?
----------------------------------------------------------------
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] [incubator-tvm] kevinthesun edited a comment on pull request #6024: [Relay][TF] Make StridedSlice support dynamic input and constant attrs
Posted by GitBox <gi...@apache.org>.
kevinthesun edited a comment on pull request #6024:
URL: https://github.com/apache/incubator-tvm/pull/6024#issuecomment-656929230
Can't topi.strided_slice support dynamic shape + const attr? What's the error? We'd better fix topi.strided_slice if there is any issue since it should be able to support dynamic input shape.
----------------------------------------------------------------
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] [incubator-tvm] lixiaoquan commented on pull request #6024: [Relay][TF] Make StridedSlice support dynamic input and constant attrs
Posted by GitBox <gi...@apache.org>.
lixiaoquan commented on pull request #6024:
URL: https://github.com/apache/incubator-tvm/pull/6024#issuecomment-676912701
> @lixiaoquan @kevinthesun What's the status on this PR? I need to split this op into the dynamic namespace and add it to the dynamic to static pass, but I don't want to conflict with what you have here, and I'd definitely appreciate the extra tests as I do that 😄
@mbrookhart Please go ahead with your change, I'm afraid there are still some work to do to handle mask conversion with dynamic shape case.
----------------------------------------------------------------
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] [incubator-tvm] kevinthesun commented on pull request #6024: [Relay][TF] Make StridedSlice support dynamic input and constant attrs
Posted by GitBox <gi...@apache.org>.
kevinthesun commented on pull request #6024:
URL: https://github.com/apache/incubator-tvm/pull/6024#issuecomment-676555312
@lixiaoquan Can you update tf frontend part?
----------------------------------------------------------------
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] [incubator-tvm] zhiics commented on pull request #6024: [Relay][TF] Make StridedSlice support dynamic input and constant attrs
Posted by GitBox <gi...@apache.org>.
zhiics commented on pull request #6024:
URL: https://github.com/apache/incubator-tvm/pull/6024#issuecomment-656264510
cc @yongwww
----------------------------------------------------------------
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] [incubator-tvm] kevinthesun commented on a change in pull request #6024: [Relay][TF] Make StridedSlice support dynamic input and constant attrs
Posted by GitBox <gi...@apache.org>.
kevinthesun commented on a change in pull request #6024:
URL: https://github.com/apache/incubator-tvm/pull/6024#discussion_r465948305
##########
File path: src/relay/op/tensor/transform.cc
##########
@@ -2146,7 +2146,18 @@ Array<te::Tensor> StridedSliceCompute(const Attrs& attrs, const Array<te::Tensor
const Type& out_type) {
const StridedSliceAttrs* param = attrs.as<StridedSliceAttrs>();
CHECK(param != nullptr);
- if (param->begin && param->end && param->strides) {
+
+ bool dyn = false;
+ for (auto& v : out_type.as<TensorTypeNode>()->shape) {
+ if (const tir::VarNode* var_node = v.as<tir::VarNode>()) {
+ if (var_node->name_hint == "any_dim") {
+ dyn = true;
+ break;
+ }
+ }
+ }
+
+ if (param->begin && param->end && param->strides && !dyn) {
Review comment:
Yeah. I think it would be more complicated to fix topi. Probably we can use dynamic stridedslice compute.
----------------------------------------------------------------
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] [incubator-tvm] kevinthesun commented on pull request #6024: [Relay][TF] Make StridedSlice support dynamic input and constant attrs
Posted by GitBox <gi...@apache.org>.
kevinthesun commented on pull request #6024:
URL: https://github.com/apache/incubator-tvm/pull/6024#issuecomment-656929230
I think this case is already supported and we don't need this change, since dynamic input shape + constant attrs can be supported by topi.strided_slice. There has already been a test case in test_any.py.
----------------------------------------------------------------
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] [incubator-tvm] kevinthesun edited a comment on pull request #6024: [Relay][TF] Make StridedSlice support dynamic input and constant attrs
Posted by GitBox <gi...@apache.org>.
kevinthesun edited a comment on pull request #6024:
URL: https://github.com/apache/incubator-tvm/pull/6024#issuecomment-656929230
Can topi.strided_slice support dynamic shape + const attr? What's the error?
----------------------------------------------------------------
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] [incubator-tvm] lixiaoquan commented on pull request #6024: [Relay][TF] Make StridedSlice support dynamic input and constant attrs
Posted by GitBox <gi...@apache.org>.
lixiaoquan commented on pull request #6024:
URL: https://github.com/apache/incubator-tvm/pull/6024#issuecomment-687462095
> @lixiaoquan Any update?
Sorry the late reply, I think I need some time to figure out how to handle mask in dynamic case, I'll close it for now.
----------------------------------------------------------------
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] [incubator-tvm] kevinthesun commented on pull request #6024: [Relay][TF] Make StridedSlice support dynamic input and constant attrs
Posted by GitBox <gi...@apache.org>.
kevinthesun commented on pull request #6024:
URL: https://github.com/apache/incubator-tvm/pull/6024#issuecomment-686053499
@lixiaoquan Any update?
----------------------------------------------------------------
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] [incubator-tvm] kevinthesun commented on a change in pull request #6024: [Relay][TF] Make StridedSlice support dynamic input and constant attrs
Posted by GitBox <gi...@apache.org>.
kevinthesun commented on a change in pull request #6024:
URL: https://github.com/apache/incubator-tvm/pull/6024#discussion_r469430653
##########
File path: python/tvm/relay/frontend/tensorflow.py
##########
@@ -1458,6 +1458,15 @@ def _impl(inputs, attr, params, mod):
return ret
+ def _dyn():
+ for d in data_shape:
+ if not isinstance(d, int):
+ return True
+ return False
+
+ if _dyn():
Review comment:
We can use shape_of to get data shape. In this case all shape dim will be relay expression and transform mask should be able to handle them. Anyway I don't think we should silently skip it since the output can be wrong and cause latter type infer error.
----------------------------------------------------------------
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] [incubator-tvm] kevinthesun commented on a change in pull request #6024: [Relay][TF] Make StridedSlice support dynamic input and constant attrs
Posted by GitBox <gi...@apache.org>.
kevinthesun commented on a change in pull request #6024:
URL: https://github.com/apache/incubator-tvm/pull/6024#discussion_r453119255
##########
File path: src/relay/op/tensor/transform.cc
##########
@@ -2146,7 +2146,18 @@ Array<te::Tensor> StridedSliceCompute(const Attrs& attrs, const Array<te::Tensor
const Type& out_type) {
const StridedSliceAttrs* param = attrs.as<StridedSliceAttrs>();
CHECK(param != nullptr);
- if (param->begin && param->end && param->strides) {
+
+ bool dyn = false;
+ for (auto& v : out_type.as<TensorTypeNode>()->shape) {
+ if (const tir::VarNode* var_node = v.as<tir::VarNode>()) {
+ if (var_node->name_hint == "any_dim") {
+ dyn = true;
+ break;
+ }
+ }
+ }
+
+ if (param->begin && param->end && param->strides && !dyn) {
Review comment:
I printed output_shape in topi::strided_slice and it is (0, 0) for that modified test_any case, which causes an runtime error. Maybe we can fix this bug directly in topi::strided_slice.
----------------------------------------------------------------
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] [incubator-tvm] mbrookhart commented on pull request #6024: [Relay][TF] Make StridedSlice support dynamic input and constant attrs
Posted by GitBox <gi...@apache.org>.
mbrookhart commented on pull request #6024:
URL: https://github.com/apache/incubator-tvm/pull/6024#issuecomment-656250654
:+1: as a fix for the current design.
In terms of improving design, I think the issue, and the reason we have two strided slice ops here, might be that the topi op for strided slice calculates the output shape. If we extract that from the topi op, we might not need to work about this complexity so much?
----------------------------------------------------------------
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] [incubator-tvm] mbrookhart commented on pull request #6024: [Relay][TF] Make StridedSlice support dynamic input and constant attrs
Posted by GitBox <gi...@apache.org>.
mbrookhart commented on pull request #6024:
URL: https://github.com/apache/incubator-tvm/pull/6024#issuecomment-675585523
@lixiaoquan @kevinthesun What's the status on this PR? I need to split this op into the dynamic namespace and add it to the dynamic to static pass, but I don't want to conflict with what you have here, and I'd definitely appreciate the extra tests as I do that :smile:
----------------------------------------------------------------
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] [incubator-tvm] kevinthesun commented on a change in pull request #6024: [Relay][TF] Make StridedSlice support dynamic input and constant attrs
Posted by GitBox <gi...@apache.org>.
kevinthesun commented on a change in pull request #6024:
URL: https://github.com/apache/incubator-tvm/pull/6024#discussion_r479490473
##########
File path: python/tvm/relay/frontend/tensorflow.py
##########
@@ -1458,6 +1458,15 @@ def _impl(inputs, attr, params, mod):
return ret
+ def _dyn():
+ for d in data_shape:
+ if not isinstance(d, int):
+ return True
+ return False
+
+ if _dyn():
Review comment:
@lixiaoquan Can you simply raise an error here and add a TODO? We can merge this PR so that backend changes can take effect.
----------------------------------------------------------------
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] [incubator-tvm] lixiaoquan closed pull request #6024: [Relay][TF] Make StridedSlice support dynamic input and constant attrs
Posted by GitBox <gi...@apache.org>.
lixiaoquan closed pull request #6024:
URL: https://github.com/apache/incubator-tvm/pull/6024
----------------------------------------------------------------
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] [incubator-tvm] lixiaoquan commented on a change in pull request #6024: [Relay][TF] Make StridedSlice support dynamic input and constant attrs
Posted by GitBox <gi...@apache.org>.
lixiaoquan commented on a change in pull request #6024:
URL: https://github.com/apache/incubator-tvm/pull/6024#discussion_r453138358
##########
File path: src/relay/op/tensor/transform.cc
##########
@@ -2146,7 +2146,18 @@ Array<te::Tensor> StridedSliceCompute(const Attrs& attrs, const Array<te::Tensor
const Type& out_type) {
const StridedSliceAttrs* param = attrs.as<StridedSliceAttrs>();
CHECK(param != nullptr);
- if (param->begin && param->end && param->strides) {
+
+ bool dyn = false;
+ for (auto& v : out_type.as<TensorTypeNode>()->shape) {
+ if (const tir::VarNode* var_node = v.as<tir::VarNode>()) {
+ if (var_node->name_hint == "any_dim") {
+ dyn = true;
+ break;
+ }
+ }
+ }
+
+ if (param->begin && param->end && param->strides && !dyn) {
Review comment:
topi::strided_slice requires static shape because it will get value from each dim. Should we change this requirement or just use DynamicStrideSlice?
```
diff --git a/topi/include/topi/transform.h b/topi/include/topi/transform.h
index b5fc02ae7..329a62ce7 100644
--- a/topi/include/topi/transform.h
+++ b/topi/include/topi/transform.h
@@ -610,6 +610,7 @@ inline Tensor strided_slice(const Tensor& x, const Array<Integer>& begin, const
for (size_t i = 0; i < src_tensor_dim; ++i) {
int64_t begin_range = stride_vec[i] < 0 ? -1 : 0;
int64_t dim_i = GetConstInt(x->shape[i]);
+ LOG(INFO) << dim_i; // it is -1 for modified case
int64_t end_range = stride_vec[i] < 0 ? dim_i - 1 : dim_i;
// transform negative indices to positive value, clips on the correct range
auto index_canonicalization = [dim_i, begin_range, end_range](int64_t index) {
@@ -635,6 +636,8 @@ inline Tensor strided_slice(const Tensor& x, const Array<Integer>& begin, const
out_shape.push_back(slice_size);
}
+// LOG(INFO) << out_shape;
+
return compute(
out_shape,
[&](const Array<Var>& indices) {
```
----------------------------------------------------------------
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] [incubator-tvm] lixiaoquan commented on a change in pull request #6024: [Relay][TF] Make StridedSlice support dynamic input and constant attrs
Posted by GitBox <gi...@apache.org>.
lixiaoquan commented on a change in pull request #6024:
URL: https://github.com/apache/incubator-tvm/pull/6024#discussion_r469071731
##########
File path: python/tvm/relay/frontend/tensorflow.py
##########
@@ -1458,6 +1458,15 @@ def _impl(inputs, attr, params, mod):
return ret
+ def _dyn():
+ for d in data_shape:
+ if not isinstance(d, int):
+ return True
+ return False
+
+ if _dyn():
Review comment:
Because mask transformation needs a concrete shape which doesn't work for dynamic shape input. I think there is more to do to handle mask in dynamic case but I'm afraid it can't support all mask transformation without concrete shape.
----------------------------------------------------------------
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