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 2022/04/18 21:22:07 UTC
[GitHub] [tvm] CircleSpin opened a new pull request, #11047: [ONNX] Reshape op
CircleSpin opened a new pull request, #11047:
URL: https://github.com/apache/tvm/pull/11047
It's been a journey.
@jwfromm @MargaretQian @anwang2009 @sfvaroglu
--
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: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] sfvaroglu commented on pull request #11047: [ONNX] Reshape op
Posted by GitBox <gi...@apache.org>.
sfvaroglu commented on PR #11047:
URL: https://github.com/apache/tvm/pull/11047#issuecomment-1101850446
@AndrewZhaoLuo mind taking a look?
--
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: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] CircleSpin commented on a diff in pull request #11047: [ONNX] Reshape op
Posted by GitBox <gi...@apache.org>.
CircleSpin commented on code in PR #11047:
URL: https://github.com/apache/tvm/pull/11047#discussion_r852454098
##########
include/tvm/topi/transform.h:
##########
@@ -321,6 +321,11 @@ inline Tensor reshape(const Tensor& x, Array<PrimExpr> newshape, std::string nam
auto x_shape = x->shape;
Array<PrimExpr> target_shape;
+ // if newshape is an empty tensor make it [0] instead.
+ if (newshape.empty()) {
+ newshape.push_back(0);
+ }
Review Comment:
:+1:
--
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: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] AndrewZhaoLuo commented on pull request #11047: [ONNX] Reshape op
Posted by GitBox <gi...@apache.org>.
AndrewZhaoLuo commented on PR #11047:
URL: https://github.com/apache/tvm/pull/11047#issuecomment-1104732545
Ill look closer tomorrow, for now some initial stuff
--
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: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] jwfromm commented on a diff in pull request #11047: [ONNX] Reshape op
Posted by GitBox <gi...@apache.org>.
jwfromm commented on code in PR #11047:
URL: https://github.com/apache/tvm/pull/11047#discussion_r855630242
##########
python/tvm/relay/op/dyn/_transform.py:
##########
@@ -45,12 +45,15 @@ def _reshape_shape_func_input_data(data_shape, newshape, ndim):
for i in const_range(len(newshape)):
if skip > 0:
skip -= 1
- elif newshape[i] > 0:
+ elif newshape[i] >= 0:
out[dst_idx] = int64(newshape[i])
src_idx += 1
dst_idx += 1
elif newshape[i] == 0:
Review Comment:
Yes you're right, I think this is a mistake that got left over during debugging.
--
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: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] jwfromm commented on pull request #11047: [ONNX] Reshape op
Posted by GitBox <gi...@apache.org>.
jwfromm commented on PR #11047:
URL: https://github.com/apache/tvm/pull/11047#issuecomment-1113900810
Thanks for making this change @CircleSpin! The PR is now merged.
--
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: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] CircleSpin commented on a diff in pull request #11047: [ONNX] Reshape op
Posted by GitBox <gi...@apache.org>.
CircleSpin commented on code in PR #11047:
URL: https://github.com/apache/tvm/pull/11047#discussion_r852453933
##########
src/relay/op/tensor/transform.cc:
##########
@@ -634,11 +636,17 @@ Array<IndexExpr> InferNewShape(const Array<IndexExpr>& data_shape, const Attrs&
oshape.push_back(newshape[i]);
++src_idx;
} else if (svalue == 0) {
- // keep same
- ICHECK_LT(src_idx, ishape.size());
- used_input_dims.insert(src_idx);
- used_output_dims.insert(oshape.size());
- oshape.push_back(ishape[src_idx++]);
+ if (allowzero) {
+ // keep same as > 0
Review Comment:
Gave it a quick rewrite, thoughts?
##########
tests/python/frontend/onnx/test_forward.py:
##########
@@ -5077,7 +5077,7 @@ def verify_eyelike(indata):
"test_reduce_sum_keepdims_random",
"test_reduce_sum_negative_axes_keepdims_example",
"test_reduce_sum_negative_axes_keepdims_random",
- "test_reshape_allowzero_reordered",
+ # "test_reshape_allowzero_reordered",
Review Comment:
bye bye test :wave:
--
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: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] CircleSpin commented on a diff in pull request #11047: [ONNX] Reshape op
Posted by GitBox <gi...@apache.org>.
CircleSpin commented on code in PR #11047:
URL: https://github.com/apache/tvm/pull/11047#discussion_r852453104
##########
src/relay/op/make_op.h:
##########
@@ -67,7 +67,7 @@ Expr MakeReduce(Expr data, Array<Integer> axis, bool keepdims, bool exclude, Str
Expr MakeRepeat(Expr data, int repeats, int axis);
-Expr MakeReshape(Expr data, Array<Integer> newshape);
+Expr MakeReshape(Expr data, Array<Integer> newshape, int allowzero = 0);
Review Comment:
int it always best to be consistent? so booliant (brilliant) :P
--
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: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] jwfromm merged pull request #11047: [ONNX] Reshape op
Posted by GitBox <gi...@apache.org>.
jwfromm merged PR #11047:
URL: https://github.com/apache/tvm/pull/11047
--
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: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] AndrewZhaoLuo commented on pull request #11047: [ONNX] Reshape op
Posted by GitBox <gi...@apache.org>.
AndrewZhaoLuo commented on PR #11047:
URL: https://github.com/apache/tvm/pull/11047#issuecomment-1102920204
I'll take a look tomorrow. This looks potentially like one that will require attention
--
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: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] jwfromm commented on a diff in pull request #11047: [ONNX] Reshape op
Posted by GitBox <gi...@apache.org>.
jwfromm commented on code in PR #11047:
URL: https://github.com/apache/tvm/pull/11047#discussion_r852434782
##########
tests/python/frontend/onnx/test_forward.py:
##########
@@ -5077,7 +5077,7 @@ def verify_eyelike(indata):
"test_reduce_sum_keepdims_random",
"test_reduce_sum_negative_axes_keepdims_example",
"test_reduce_sum_negative_axes_keepdims_random",
- "test_reshape_allowzero_reordered",
+ # "test_reshape_allowzero_reordered",
Review Comment:
I think we probably want to just delete this line instead of commenting it out.
##########
src/relay/op/tensor/transform.cc:
##########
@@ -634,11 +636,17 @@ Array<IndexExpr> InferNewShape(const Array<IndexExpr>& data_shape, const Attrs&
oshape.push_back(newshape[i]);
++src_idx;
} else if (svalue == 0) {
- // keep same
- ICHECK_LT(src_idx, ishape.size());
- used_input_dims.insert(src_idx);
- used_output_dims.insert(oshape.size());
- oshape.push_back(ishape[src_idx++]);
+ if (allowzero) {
+ // keep same as > 0
Review Comment:
This comment could be clearer, especially since "keep same" was used to describe the allowzero = false condition :)
##########
src/relay/op/make_op.h:
##########
@@ -67,7 +67,7 @@ Expr MakeReduce(Expr data, Array<Integer> axis, bool keepdims, bool exclude, Str
Expr MakeRepeat(Expr data, int repeats, int axis);
-Expr MakeReshape(Expr data, Array<Integer> newshape);
+Expr MakeReshape(Expr data, Array<Integer> newshape, int allowzero = 0);
Review Comment:
You use `bool` for allowzero in the dynamic `MakeReshape`. Let's also use it here (and in transform.cc) instead of `int` for consistency.
##########
include/tvm/topi/transform.h:
##########
@@ -321,6 +321,11 @@ inline Tensor reshape(const Tensor& x, Array<PrimExpr> newshape, std::string nam
auto x_shape = x->shape;
Array<PrimExpr> target_shape;
+ // if newshape is an empty tensor make it [0] instead.
+ if (newshape.empty()) {
+ newshape.push_back(0);
+ }
Review Comment:
I think we decided this snippet is actually not needed. Can you remove it and confirm everything still works?
##########
include/tvm/topi/detail/ravel_unravel.h:
##########
@@ -70,8 +70,12 @@ inline Array<PrimExpr> UnravelIndex(PrimExpr idx, Array<PrimExpr> shape) {
std::vector<PrimExpr> indices;
for (int i = static_cast<int>(shape.size()) - 1; i >= 0; --i) {
- indices.push_back(indexmod(idx, shape[i]));
- idx = indexdiv(idx, shape[i]);
+ if (shape[i].as<IntImmNode>()->value == 0) {
Review Comment:
I just wanted to call out this bit for other reviewers. To support empty tensor reshapes we have to insert a check in the unravel inner loop. This could affect performance but is the only way to enable the functionality. Does anyone have any strong thoughts on this approach?
--
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: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] AndrewZhaoLuo commented on a diff in pull request #11047: [ONNX] Reshape op
Posted by GitBox <gi...@apache.org>.
AndrewZhaoLuo commented on code in PR #11047:
URL: https://github.com/apache/tvm/pull/11047#discussion_r854787543
##########
python/tvm/relay/frontend/onnx.py:
##########
@@ -1203,11 +1203,14 @@ def _impl_v1(cls, inputs, attr, params):
@classmethod
def _impl_v5(cls, inputs, attr, params):
+ allowzero = False
Review Comment:
suggest attr.get('allowzero', False)
##########
tests/python/contrib/test_cmsisnn/utils.py:
##########
@@ -290,7 +290,7 @@ def generate_ref_data_tflite(model):
def create_conv2d_tflite_model(ifm_shape, kernel_shape, strides, dilation, padding, activation):
- """ This method prepares TFlite graph with a single Conv2d layer """
Review Comment:
probably undo this change to not gum up the commit log
##########
python/tvm/script/tir/__init__.pyi:
##########
@@ -226,6 +226,7 @@ def alloc_buffer(
"""
special_stmt - Reads/Writes
"""
+
Review Comment:
probably undo this change to not gum up the commit log
##########
python/tvm/relay/op/dyn/_transform.py:
##########
@@ -45,12 +45,15 @@ def _reshape_shape_func_input_data(data_shape, newshape, ndim):
for i in const_range(len(newshape)):
if skip > 0:
skip -= 1
- elif newshape[i] > 0:
+ elif newshape[i] >= 0:
out[dst_idx] = int64(newshape[i])
src_idx += 1
dst_idx += 1
elif newshape[i] == 0:
Review Comment:
won't this codepath not be hit now? this seems a little sus (since above is >= 0 now). That seems incorrect to cover the case where allowzero != 0
##########
python/tvm/relay/op/dyn/_transform.py:
##########
@@ -45,12 +45,15 @@ def _reshape_shape_func_input_data(data_shape, newshape, ndim):
for i in const_range(len(newshape)):
if skip > 0:
skip -= 1
- elif newshape[i] > 0:
+ elif newshape[i] >= 0:
out[dst_idx] = int64(newshape[i])
src_idx += 1
dst_idx += 1
elif newshape[i] == 0:
Review Comment:
Isn't > 0 the comparison we want?
--
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: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] AndrewZhaoLuo commented on a diff in pull request #11047: [ONNX] Reshape op
Posted by GitBox <gi...@apache.org>.
AndrewZhaoLuo commented on code in PR #11047:
URL: https://github.com/apache/tvm/pull/11047#discussion_r855715674
##########
include/tvm/topi/detail/ravel_unravel.h:
##########
@@ -70,8 +70,12 @@ inline Array<PrimExpr> UnravelIndex(PrimExpr idx, Array<PrimExpr> shape) {
std::vector<PrimExpr> indices;
for (int i = static_cast<int>(shape.size()) - 1; i >= 0; --i) {
- indices.push_back(indexmod(idx, shape[i]));
- idx = indexdiv(idx, shape[i]);
+ if (shape[i].as<IntImmNode>()->value == 0) {
Review Comment:
this change is gone now right?
--
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: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org