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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
