[GitHub] [tvm] jwfromm commented on a diff in pull request #11047: [ONNX] Reshape op

[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