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/09/08 23:54:22 UTC
[GitHub] [tvm] AndrewZhaoLuo opened a new pull request #8971: [Relay][Topi][Op][Onnx] Support negative indices in gather
AndrewZhaoLuo opened a new pull request #8971:
URL: https://github.com/apache/tvm/pull/8971
Supports negative indices for gather op. This was what caused issues https://github.com/apache/tvm/issues/8918 and https://github.com/apache/tvm/issues/8964.
This should fix the flaky onnx test and allow for the remaining non-expanded onnx tests
Need to add gathernd support similarly.
Testing:
Ran `pytest tests/python/frontend/onnx/test_forward.py::test_onnx_nodes -k test_nllloss_NCd1d2d3_none_no_weight_negative_ii --count=100` and confirmed no failures
--
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] jroesch commented on pull request #8971: [Onnx] Fix NLL Loss tests
Posted by GitBox <gi...@apache.org>.
jroesch commented on pull request #8971:
URL: https://github.com/apache/tvm/pull/8971#issuecomment-920701813
@masahi can you land this one if you are OK?
--
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 #8971: [WIP][Relay][Topi][Op][Onnx] Support negative indices in gather
Posted by GitBox <gi...@apache.org>.
AndrewZhaoLuo commented on pull request #8971:
URL: https://github.com/apache/tvm/pull/8971#issuecomment-916660390
> @AndrewZhaoLuo Maybe we can remove this function?
> https://github.com/apache/tvm/blob/cf439ec7e73ba2bee3bb58ae1b90a543beb0ee76/python/tvm/relay/frontend/onnx.py#L1510-L1511
I need to replace "take" so I'll put it in another PR.
--
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] masahi commented on a change in pull request #8971: [Relay][Topi][Op][Onnx] Support negative indices in gather, gathernd. Fixes NLL Loss tests
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #8971:
URL: https://github.com/apache/tvm/pull/8971#discussion_r707850237
##########
File path: src/te/tensor.cc
##########
@@ -39,15 +39,26 @@ IterVar reduce_axis(Range dom, std::string name) { return IterVar(dom, Var(name)
Var var(std::string name_hint, DataType t) { return Var(name_hint, t); }
// Tensor
-PrimExpr Tensor::operator()(Array<Var> indices) const {
+PrimExpr Tensor::operator()(Array<Var> indices, bool support_negative_indices) const {
Array<PrimExpr> arr(indices.begin(), indices.end());
- return operator()(arr);
+ return operator()(arr, support_negative_indices);
}
-PrimExpr Tensor::operator()(Array<PrimExpr> indices) const {
- if (ndim() != 0) {
- ICHECK_EQ(ndim(), indices.size()) << "Tensor dimension mismatch in read "
- << "ndim = " << ndim() << ", indices.size=" << indices.size();
+PrimExpr Tensor::operator()(Array<PrimExpr> indices, bool support_negative_indices) const {
+ Array<PrimExpr> shape = (*this)->shape;
+
+ if (shape.size() != 0) {
+ ICHECK_EQ(shape.size(), indices.size())
+ << "Tensor dimension mismatch in read "
+ << "ndim = " << ndim() << ", indices.size=" << indices.size();
+ }
+
+ if (support_negative_indices) {
+ for (size_t i = 0; i < shape.size(); i++) {
+ PrimExpr new_index = if_then_else(indices[i] < make_const(indices[i]->dtype, 0),
+ indices[i] + shape[i], indices[i]);
+ indices.Set(i, new_index);
Review comment:
Negative indices handling is also done in
https://github.com/apache/tvm/blob/d9fe67259ead828f174e0be719906e3f2d3c2137/python/tvm/relay/op/transform.py#L926-L927
https://github.com/apache/tvm/blob/cbe3dcad5e3f9358af8d2d79e2880f92718a5d0b/include/tvm/topi/detail/strided_slice.h#L45-L48
https://github.com/apache/tvm/blob/cbe3dcad5e3f9358af8d2d79e2880f92718a5d0b/include/tvm/topi/detail/strided_slice.h#L105
I believe there are other cases like this spread across the code base. Maybe we should revisit all index-taking op and centralize negative indices handling. Generally I think people prefer not making a change down the stack.
--
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] mbrookhart commented on pull request #8971: [Relay][Topi][Op][Onnx] Support negative indices in gather, gathernd. Fixes NLL Loss tests
Posted by GitBox <gi...@apache.org>.
mbrookhart commented on pull request #8971:
URL: https://github.com/apache/tvm/pull/8971#issuecomment-918312739
@AndrewZhaoLuo, can you try profiling that with a larger 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.
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 #8971: [Relay][Topi][Op][Onnx] Support negative indices in gather, gathernd. Fixes NLL Loss tests
Posted by GitBox <gi...@apache.org>.
AndrewZhaoLuo commented on pull request #8971:
URL: https://github.com/apache/tvm/pull/8971#issuecomment-918423057
> This failed the dynamic gather test in test_any
Should be fixed now. In general it seems people are inconsistent with using kwargs in the codebase.
--
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 #8971: [Relay][Topi][Op][Onnx] Support negative indices in gather, gathernd
Posted by GitBox <gi...@apache.org>.
AndrewZhaoLuo commented on pull request #8971:
URL: https://github.com/apache/tvm/pull/8971#issuecomment-917228199
This is ready for a full review 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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi commented on pull request #8971: [WIP][Relay][Topi][Op][Onnx] Support negative indices in gather
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #8971:
URL: https://github.com/apache/tvm/pull/8971#issuecomment-915658238
@AndrewZhaoLuo Maybe we can remove this function?
https://github.com/apache/tvm/blob/cf439ec7e73ba2bee3bb58ae1b90a543beb0ee76/python/tvm/relay/frontend/onnx.py#L1510-L1511
--
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] masahi merged pull request #8971: [Onnx] Fix NLL Loss tests
Posted by GitBox <gi...@apache.org>.
masahi merged pull request #8971:
URL: https://github.com/apache/tvm/pull/8971
--
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] anwang2009 commented on a change in pull request #8971: [WIP][Relay][Topi][Op][Onnx] Support negative indices in gather
Posted by GitBox <gi...@apache.org>.
anwang2009 commented on a change in pull request #8971:
URL: https://github.com/apache/tvm/pull/8971#discussion_r706476650
##########
File path: tests/python/relay/test_op_level3.py
##########
@@ -1241,13 +1238,16 @@ def test_scatter_add(self, target, dev, ref_data, dshape, ishape, axis, dtype):
],
)
def test_gather(target, dev, executor_kind, data, axis, indices, ref_res):
- def verify_gather(data, axis, indices, ref_res):
+ def verify_gather(data, axis, indices, ref_res, check_negative=False):
Review comment:
Seems like check_negative is never set to anything other than False with the current tests. remove?
--
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] masahi commented on a change in pull request #8971: [Relay][Topi][Op][Onnx] Support negative indices in gather, gathernd. Fixes NLL Loss tests
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #8971:
URL: https://github.com/apache/tvm/pull/8971#discussion_r708648108
##########
File path: src/te/tensor.cc
##########
@@ -39,15 +39,26 @@ IterVar reduce_axis(Range dom, std::string name) { return IterVar(dom, Var(name)
Var var(std::string name_hint, DataType t) { return Var(name_hint, t); }
// Tensor
-PrimExpr Tensor::operator()(Array<Var> indices) const {
+PrimExpr Tensor::operator()(Array<Var> indices, bool support_negative_indices) const {
Array<PrimExpr> arr(indices.begin(), indices.end());
- return operator()(arr);
+ return operator()(arr, support_negative_indices);
}
-PrimExpr Tensor::operator()(Array<PrimExpr> indices) const {
- if (ndim() != 0) {
- ICHECK_EQ(ndim(), indices.size()) << "Tensor dimension mismatch in read "
- << "ndim = " << ndim() << ", indices.size=" << indices.size();
+PrimExpr Tensor::operator()(Array<PrimExpr> indices, bool support_negative_indices) const {
+ Array<PrimExpr> shape = (*this)->shape;
+
+ if (shape.size() != 0) {
+ ICHECK_EQ(shape.size(), indices.size())
+ << "Tensor dimension mismatch in read "
+ << "ndim = " << ndim() << ", indices.size=" << indices.size();
+ }
+
+ if (support_negative_indices) {
+ for (size_t i = 0; i < shape.size(); i++) {
+ PrimExpr new_index = if_then_else(indices[i] < make_const(indices[i]->dtype, 0),
+ indices[i] + shape[i], indices[i]);
+ indices.Set(i, new_index);
Review comment:
Yeah I agree that implementation-wise, this is more convenient. Since this is a fundamental data structure change, how about we open a separate PR for negative indexing support to `te::Tensor`, to get opinions from more people?
--
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] masahi commented on a change in pull request #8971: [Relay][Topi][Op][Onnx] Support negative indices in gather, gathernd. Fixes NLL Loss tests
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #8971:
URL: https://github.com/apache/tvm/pull/8971#discussion_r707846211
##########
File path: include/tvm/topi/transform.h
##########
@@ -1242,6 +1244,8 @@ inline Tensor gather(const Tensor& data, int axis, const Tensor& indices,
out_shape.push_back(indices->shape[i]);
}
+ PrimExpr axis_size = data->shape[axis];
Review comment:
remove it
##########
File path: include/tvm/topi/transform.h
##########
@@ -1252,12 +1256,13 @@ inline Tensor gather(const Tensor& data, int axis, const Tensor& indices,
Array<PrimExpr> real_indices;
for (size_t i = 0; i < ndim_i; ++i) {
if (i == static_cast<size_t>(axis)) {
- real_indices.push_back(indices(indices_position));
+ PrimExpr index = indices(indices_position);
+ real_indices.push_back(index);
Review comment:
remove this diff
--
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] masahi commented on a change in pull request #8971: [Relay][Topi][Op][Onnx] Support negative indices in gather, gathernd. Fixes NLL Loss tests
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #8971:
URL: https://github.com/apache/tvm/pull/8971#discussion_r707850237
##########
File path: src/te/tensor.cc
##########
@@ -39,15 +39,26 @@ IterVar reduce_axis(Range dom, std::string name) { return IterVar(dom, Var(name)
Var var(std::string name_hint, DataType t) { return Var(name_hint, t); }
// Tensor
-PrimExpr Tensor::operator()(Array<Var> indices) const {
+PrimExpr Tensor::operator()(Array<Var> indices, bool support_negative_indices) const {
Array<PrimExpr> arr(indices.begin(), indices.end());
- return operator()(arr);
+ return operator()(arr, support_negative_indices);
}
-PrimExpr Tensor::operator()(Array<PrimExpr> indices) const {
- if (ndim() != 0) {
- ICHECK_EQ(ndim(), indices.size()) << "Tensor dimension mismatch in read "
- << "ndim = " << ndim() << ", indices.size=" << indices.size();
+PrimExpr Tensor::operator()(Array<PrimExpr> indices, bool support_negative_indices) const {
+ Array<PrimExpr> shape = (*this)->shape;
+
+ if (shape.size() != 0) {
+ ICHECK_EQ(shape.size(), indices.size())
+ << "Tensor dimension mismatch in read "
+ << "ndim = " << ndim() << ", indices.size=" << indices.size();
+ }
+
+ if (support_negative_indices) {
+ for (size_t i = 0; i < shape.size(); i++) {
+ PrimExpr new_index = if_then_else(indices[i] < make_const(indices[i]->dtype, 0),
+ indices[i] + shape[i], indices[i]);
+ indices.Set(i, new_index);
Review comment:
Negative indices handling is also done in https://github.com/apache/tvm/blob/d9fe67259ead828f174e0be719906e3f2d3c2137/python/tvm/relay/op/transform.py#L926-L927
I believe there are other cases like this spread across the code base. Maybe we should revisit all index-taking op and centralize negative indices handling. Generally I think people prefer not making a change down the stack.
--
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 change in pull request #8971: [Relay][Topi][Op][Onnx] Support negative indices in gather, gathernd. Fixes NLL Loss tests
Posted by GitBox <gi...@apache.org>.
AndrewZhaoLuo commented on a change in pull request #8971:
URL: https://github.com/apache/tvm/pull/8971#discussion_r708651820
##########
File path: src/te/tensor.cc
##########
@@ -39,15 +39,26 @@ IterVar reduce_axis(Range dom, std::string name) { return IterVar(dom, Var(name)
Var var(std::string name_hint, DataType t) { return Var(name_hint, t); }
// Tensor
-PrimExpr Tensor::operator()(Array<Var> indices) const {
+PrimExpr Tensor::operator()(Array<Var> indices, bool support_negative_indices) const {
Array<PrimExpr> arr(indices.begin(), indices.end());
- return operator()(arr);
+ return operator()(arr, support_negative_indices);
}
-PrimExpr Tensor::operator()(Array<PrimExpr> indices) const {
- if (ndim() != 0) {
- ICHECK_EQ(ndim(), indices.size()) << "Tensor dimension mismatch in read "
- << "ndim = " << ndim() << ", indices.size=" << indices.size();
+PrimExpr Tensor::operator()(Array<PrimExpr> indices, bool support_negative_indices) const {
+ Array<PrimExpr> shape = (*this)->shape;
+
+ if (shape.size() != 0) {
+ ICHECK_EQ(shape.size(), indices.size())
+ << "Tensor dimension mismatch in read "
+ << "ndim = " << ndim() << ", indices.size=" << indices.size();
+ }
+
+ if (support_negative_indices) {
+ for (size_t i = 0; i < shape.size(); i++) {
+ PrimExpr new_index = if_then_else(indices[i] < make_const(indices[i]->dtype, 0),
+ indices[i] + shape[i], indices[i]);
+ indices.Set(i, new_index);
Review comment:
I think that's a fair point. I'll refactor this to use `normalize_gather_indices()` in the meantime and do as you say.
--
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 change in pull request #8971: [Relay][Topi][Op][Onnx] Support negative indices in gather
Posted by GitBox <gi...@apache.org>.
AndrewZhaoLuo commented on a change in pull request #8971:
URL: https://github.com/apache/tvm/pull/8971#discussion_r706482284
##########
File path: tests/python/relay/test_op_level3.py
##########
@@ -1241,13 +1238,16 @@ def test_scatter_add(self, target, dev, ref_data, dshape, ishape, axis, dtype):
],
)
def test_gather(target, dev, executor_kind, data, axis, indices, ref_res):
- def verify_gather(data, axis, indices, ref_res):
+ def verify_gather(data, axis, indices, ref_res, check_negative=False):
Review comment:
Nope, one of the tests is supposed to check negatives, thanks for the catch
--
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 #8971: [Onnx] Fix NLL Loss tests
Posted by GitBox <gi...@apache.org>.
AndrewZhaoLuo commented on pull request #8971:
URL: https://github.com/apache/tvm/pull/8971#issuecomment-920304017
Ok folks, I've removed the controversial changes and did an alternate work around. PTAL when you have 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.
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 edited a comment on pull request #8971: [Relay][Topi][Op][Onnx] Support negative indices in gather, gathernd
Posted by GitBox <gi...@apache.org>.
AndrewZhaoLuo edited a comment on pull request #8971:
URL: https://github.com/apache/tvm/pull/8971#issuecomment-917228526
Another topic of discussion is whether we should just allow negative indices to be always on. There is overhead but I believe it is very small.
I did it with an indexmod operation instead of a branch and it increased runtime of gather by 3% across 100 iterations and a relatively small input size of [3, 3, 3]
--
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 #8971: [Onnx] Fix NLL Loss tests
Posted by GitBox <gi...@apache.org>.
AndrewZhaoLuo commented on pull request #8971:
URL: https://github.com/apache/tvm/pull/8971#issuecomment-920359675
https://github.com/apache/tvm/pull/9023 <-- discussion about making negative indices simpler
--
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 #8971: [Relay][Topi][Op][Onnx] Support negative indices in gather, gathernd
Posted by GitBox <gi...@apache.org>.
AndrewZhaoLuo commented on pull request #8971:
URL: https://github.com/apache/tvm/pull/8971#issuecomment-917228526
Another topic of discussion is whether we should just allow negative indices to be always on. There is overhead but I believe it is very small.
--
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 change in pull request #8971: [Relay][Topi][Op][Onnx] Support negative indices in gather, gathernd. Fixes NLL Loss tests
Posted by GitBox <gi...@apache.org>.
AndrewZhaoLuo commented on a change in pull request #8971:
URL: https://github.com/apache/tvm/pull/8971#discussion_r708540484
##########
File path: src/te/tensor.cc
##########
@@ -39,15 +39,26 @@ IterVar reduce_axis(Range dom, std::string name) { return IterVar(dom, Var(name)
Var var(std::string name_hint, DataType t) { return Var(name_hint, t); }
// Tensor
-PrimExpr Tensor::operator()(Array<Var> indices) const {
+PrimExpr Tensor::operator()(Array<Var> indices, bool support_negative_indices) const {
Array<PrimExpr> arr(indices.begin(), indices.end());
- return operator()(arr);
+ return operator()(arr, support_negative_indices);
}
-PrimExpr Tensor::operator()(Array<PrimExpr> indices) const {
- if (ndim() != 0) {
- ICHECK_EQ(ndim(), indices.size()) << "Tensor dimension mismatch in read "
- << "ndim = " << ndim() << ", indices.size=" << indices.size();
+PrimExpr Tensor::operator()(Array<PrimExpr> indices, bool support_negative_indices) const {
+ Array<PrimExpr> shape = (*this)->shape;
+
+ if (shape.size() != 0) {
+ ICHECK_EQ(shape.size(), indices.size())
+ << "Tensor dimension mismatch in read "
+ << "ndim = " << ndim() << ", indices.size=" << indices.size();
+ }
+
+ if (support_negative_indices) {
+ for (size_t i = 0; i < shape.size(); i++) {
+ PrimExpr new_index = if_then_else(indices[i] < make_const(indices[i]->dtype, 0),
+ indices[i] + shape[i], indices[i]);
+ indices.Set(i, new_index);
Review comment:
Hmm this is a good point. I think pushing down the stack is the right choice personally since I expect the most basic indexing op to work with negative indices. Since all of the other operations will use these basic indexing ops we should therefore get these things for free. In our case, we add a flag to a basic indexing operation which turns on this features.
Otherwise we'll get a lot of copies of the same code everywhere.
--
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] masahi commented on a change in pull request #8971: [Relay][Topi][Op][Onnx] Support negative indices in gather, gathernd. Fixes NLL Loss tests
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #8971:
URL: https://github.com/apache/tvm/pull/8971#discussion_r708648108
##########
File path: src/te/tensor.cc
##########
@@ -39,15 +39,26 @@ IterVar reduce_axis(Range dom, std::string name) { return IterVar(dom, Var(name)
Var var(std::string name_hint, DataType t) { return Var(name_hint, t); }
// Tensor
-PrimExpr Tensor::operator()(Array<Var> indices) const {
+PrimExpr Tensor::operator()(Array<Var> indices, bool support_negative_indices) const {
Array<PrimExpr> arr(indices.begin(), indices.end());
- return operator()(arr);
+ return operator()(arr, support_negative_indices);
}
-PrimExpr Tensor::operator()(Array<PrimExpr> indices) const {
- if (ndim() != 0) {
- ICHECK_EQ(ndim(), indices.size()) << "Tensor dimension mismatch in read "
- << "ndim = " << ndim() << ", indices.size=" << indices.size();
+PrimExpr Tensor::operator()(Array<PrimExpr> indices, bool support_negative_indices) const {
+ Array<PrimExpr> shape = (*this)->shape;
+
+ if (shape.size() != 0) {
+ ICHECK_EQ(shape.size(), indices.size())
+ << "Tensor dimension mismatch in read "
+ << "ndim = " << ndim() << ", indices.size=" << indices.size();
+ }
+
+ if (support_negative_indices) {
+ for (size_t i = 0; i < shape.size(); i++) {
+ PrimExpr new_index = if_then_else(indices[i] < make_const(indices[i]->dtype, 0),
+ indices[i] + shape[i], indices[i]);
+ indices.Set(i, new_index);
Review comment:
Yeah I agree that implementation-wise, this is more convenient. How about we open a separate PR for negative indexing support to `te::Tensor`, to get opinions from more people?
--
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] mbrookhart commented on pull request #8971: [Relay][Topi][Op][Onnx] Support negative indices in gather, gathernd. Fixes NLL Loss tests
Posted by GitBox <gi...@apache.org>.
mbrookhart commented on pull request #8971:
URL: https://github.com/apache/tvm/pull/8971#issuecomment-918313450
This failed the dynamic gather test in test_any
--
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