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/02/02 14:55:34 UTC
[GitHub] [tvm] echuraev opened a new pull request #7391: [ONNX] Add CumSum operator to ONNX frontend
echuraev opened a new pull request #7391:
URL: https://github.com/apache/tvm/pull/7391
Thanks for contributing to TVM! Please refer to guideline https://tvm.apache.org/docs/contribute/ for useful information and tips. After the pull request is submitted, please request code reviews from [Reviewers](https://github.com/apache/incubator-tvm/blob/master/CONTRIBUTORS.md#reviewers) by @ them in the pull request thread.
----------------------------------------------------------------
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] [tvm] masahi commented on pull request #7391: [ONNX] Add CumSum operator to ONNX frontend
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #7391:
URL: https://github.com/apache/tvm/pull/7391#issuecomment-775778750
I'm happy with the current implementation. It is faster, and exclusive cumsum could be useful for other purpose (just as `exclusive_scan` in topi cuda is used in multiple places).
----------------------------------------------------------------
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] [tvm] echuraev commented on a change in pull request #7391: [ONNX] Add CumSum operator to ONNX frontend
Posted by GitBox <gi...@apache.org>.
echuraev commented on a change in pull request #7391:
URL: https://github.com/apache/tvm/pull/7391#discussion_r572600459
##########
File path: python/tvm/topi/cumsum.py
##########
@@ -75,6 +84,12 @@ def maybe_cast(x):
elif i > axis:
axis_mul_after *= value
+ if exclusive is None:
+ exclusive = 0
+
+ if reverse is None:
+ reverse = 0
+
Review comment:
You are right. Sorry, I forgot to remove it.
----------------------------------------------------------------
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] [tvm] echuraev commented on pull request #7391: [ONNX] Add CumSum operator to ONNX frontend
Posted by GitBox <gi...@apache.org>.
echuraev commented on pull request #7391:
URL: https://github.com/apache/tvm/pull/7391#issuecomment-775732937
@mbrookhart, @masahi, I have one question about implementation of `exclusive` attribute. In current implementation, I added this attribute to topi kernels, but also I could do the same thing only for ONNX frontend (by adding `subtract` operation after `cumsum`). What do you think, what is the better approach? In first case we don't have overhead of `subtract` operation, but in the second we should write less code.
----------------------------------------------------------------
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] [tvm] mbrookhart commented on pull request #7391: [ONNX] Add CumSum operator to ONNX frontend
Posted by GitBox <gi...@apache.org>.
mbrookhart commented on pull request #7391:
URL: https://github.com/apache/tvm/pull/7391#issuecomment-771932948
Oh, I just noticed it's a draft :) You're probably not ready for review, sorry.
----------------------------------------------------------------
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] [tvm] masahi commented on pull request #7391: [ONNX] Add CumSum operator to ONNX frontend
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #7391:
URL: https://github.com/apache/tvm/pull/7391#issuecomment-772568769
`exclusive` can be easily supported by `cumsum(data) - data`. Similarly, I expect `reverse` can also be supported by composing with `reverse` op. But I think this attribute is weird (why would anyone one want to do reverse cumsum), so it might not be worth it.
It's strange ONNX `Cumsum` doesn't have output dtype attribute. Without it, there could be overflow issues for int32 input. Also it wouldn't work for boolean input, which is a common input to cumsum. I guess they expect casting to be done by users beforehand.
----------------------------------------------------------------
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] [tvm] mbrookhart commented on a change in pull request #7391: [ONNX] Add CumSum operator to ONNX frontend
Posted by GitBox <gi...@apache.org>.
mbrookhart commented on a change in pull request #7391:
URL: https://github.com/apache/tvm/pull/7391#discussion_r572180871
##########
File path: python/tvm/relay/op/transform.py
##########
@@ -1368,4 +1377,8 @@ def cumsum(data, axis=None, dtype=None):
cumsum(a, dtype=int32) # dtype should be provided to get the expected results
-> [1, 1, 2, 2, 3, 4, 4]
"""
- return _make.cumsum(data, axis, dtype)
+ if rev is not None and rev != 0:
+ out = _make.reverse(data, axis)
+ out = _make.cumsum(out, axis, dtype, exclusive)
+ return _make.reverse(out, axis)
+ return _make.cumsum(data, axis, dtype, exclusive)
Review comment:
Hmm, you do reverse here, but exclusive in the topi kernel.
##########
File path: python/tvm/topi/cumsum.py
##########
@@ -75,6 +84,12 @@ def maybe_cast(x):
elif i > axis:
axis_mul_after *= value
+ if exclusive is None:
+ exclusive = 0
+
+ if reverse is None:
+ reverse = 0
+
Review comment:
You're not using reverse in the topi?
----------------------------------------------------------------
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] [tvm] masahi commented on a change in pull request #7391: [ONNX] Add CumSum operator to ONNX frontend
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #7391:
URL: https://github.com/apache/tvm/pull/7391#discussion_r572703773
##########
File path: include/tvm/relay/attrs/transform.h
##########
@@ -442,9 +442,13 @@ struct MatrixSetDiagAttrs : public tvm::AttrsNode<MatrixSetDiagAttrs> {
struct CumsumAttrs : public tvm::AttrsNode<CumsumAttrs> {
Integer axis;
DataType dtype;
+ Integer exclusive;
TVM_DECLARE_ATTRS(CumsumAttrs, "relay.attrs.CumsumAttrs") {
TVM_ATTR_FIELD(axis).describe("The axis to sum over").set_default(NullValue<Integer>());
TVM_ATTR_FIELD(dtype).describe("Output data type").set_default(NullValue<DataType>());
+ TVM_ATTR_FIELD(exclusive)
+ .describe("The top element is not included")
Review comment:
top -> first, since we don't support reverse and "top" always means "first".
----------------------------------------------------------------
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] [tvm] masahi commented on a change in pull request #7391: [ONNX] Add CumSum operator to ONNX frontend
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #7391:
URL: https://github.com/apache/tvm/pull/7391#discussion_r572704170
##########
File path: python/tvm/topi/cuda/scan.py
##########
@@ -504,6 +504,12 @@ def cumsum(data, axis=None, dtype=None):
Type of the returned array and of the accumulator in which the elements are summed.
If dtype is not specified, it defaults to the dtype of data.
+ exclusive : int, optional
+ If set to 1 will return exclusive sum in which the top element is not
Review comment:
top -> first
----------------------------------------------------------------
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] [tvm] masahi commented on pull request #7391: [ONNX] Add CumSum operator to ONNX frontend
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #7391:
URL: https://github.com/apache/tvm/pull/7391#issuecomment-775469928
I think `reverse` should be done in the frontend, since this seems ONNX specific. For `exclusive`, if you want to avoid the overhead of subtraction, we can do cumsum with exclusive scan. See https://github.com/apache/tvm/blob/1e0d3569b94f650243f4d0ac204d196e3be8b0aa/python/tvm/topi/cuda/scan.py#L517 if `exclusive=True`, we can use `exclusive_scan` instead of `inclusive_scan`.
----------------------------------------------------------------
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] [tvm] masahi commented on a change in pull request #7391: [ONNX] Add CumSum operator to ONNX frontend
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #7391:
URL: https://github.com/apache/tvm/pull/7391#discussion_r572384767
##########
File path: python/tvm/topi/cumsum.py
##########
@@ -90,6 +105,10 @@ def gen_ir(data_buf, out_buf):
cur_idx = base_idx + k * axis_mul_after
prev_idx = base_idx + (k - 1) * axis_mul_after
out_buf[cur_idx] = out_buf[prev_idx] + maybe_cast(data_buf[cur_idx])
+ if exclusive != 0:
+ with ib.for_range(0, cumsum_axis_len, "_k") as k:
+ cur_idx = base_idx + k * axis_mul_after
+ out_buf[cur_idx] = out_buf[cur_idx] - maybe_cast(data_buf[cur_idx])
Review comment:
instead of this, we can just do exclusive sum here by initializing `out_buf[base_idx]` to 0 if `exclusive != 0` at https://github.com/apache/tvm/blob/1e0d3569b94f650243f4d0ac204d196e3be8b0aa/python/tvm/topi/cumsum.py#L87 and making some index adjustment in the inner loop.
----------------------------------------------------------------
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] [tvm] echuraev commented on pull request #7391: [ONNX] Add CumSum operator to ONNX frontend
Posted by GitBox <gi...@apache.org>.
echuraev commented on pull request #7391:
URL: https://github.com/apache/tvm/pull/7391#issuecomment-772543313
@mbrookhart, thank you for you questions. Now I added error generation for `exclusive` and `reverse` attributes, but I'm planning to implement them in this 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi commented on a change in pull request #7391: [ONNX] Add CumSum operator to ONNX frontend
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #7391:
URL: https://github.com/apache/tvm/pull/7391#discussion_r572705549
##########
File path: python/tvm/topi/cumsum.py
##########
@@ -84,12 +93,18 @@ def gen_ir(data_buf, out_buf):
i = fused // axis_mul_after
j = fused % axis_mul_after
base_idx = i * cumsum_axis_len * axis_mul_after + j
- out_buf[base_idx] = maybe_cast(data_buf[base_idx])
+ if exclusive == 0:
+ out_buf[base_idx] = maybe_cast(data_buf[base_idx])
+ else:
+ out_buf[base_idx] = 0.0
Review comment:
This will likely give an error if the input is an integer tensor. Need to cast 0 to `dtype`
----------------------------------------------------------------
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] [tvm] mbrookhart commented on pull request #7391: [ONNX] Add CumSum operator to ONNX frontend
Posted by GitBox <gi...@apache.org>.
mbrookhart commented on pull request #7391:
URL: https://github.com/apache/tvm/pull/7391#issuecomment-771932948
Oh, I just noticed it's a draft :) You're probably not ready for review, sorry.
----------------------------------------------------------------
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] [tvm] masahi commented on a change in pull request #7391: [ONNX] Add CumSum operator to ONNX frontend
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #7391:
URL: https://github.com/apache/tvm/pull/7391#discussion_r572704407
##########
File path: python/tvm/topi/cumsum.py
##########
@@ -38,6 +38,12 @@ def cumsum(data, axis=None, dtype=None):
Type of the returned array and of the accumulator in which the elements are summed.
If dtype is not specified, it defaults to the dtype of data.
+ exclusive : int, optional
+ If set to 1 will return exclusive sum in which the top element is not
Review comment:
top -> first
----------------------------------------------------------------
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] [tvm] masahi commented on a change in pull request #7391: [ONNX] Add CumSum operator to ONNX frontend
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #7391:
URL: https://github.com/apache/tvm/pull/7391#discussion_r572704018
##########
File path: python/tvm/relay/op/transform.py
##########
@@ -1339,6 +1339,12 @@ def cumsum(data, axis=None, dtype=None):
Type of the returned array and of the accumulator in which the elements are summed.
If dtype is not specified, it defaults to the dtype of data.
+ exclusive : int, optional
+ If set to 1 will return exclusive sum in which the top element is not
Review comment:
top -> first
----------------------------------------------------------------
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] [tvm] mbrookhart merged pull request #7391: [ONNX] Add CumSum operator to ONNX frontend
Posted by GitBox <gi...@apache.org>.
mbrookhart merged pull request #7391:
URL: https://github.com/apache/tvm/pull/7391
----------------------------------------------------------------
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] [tvm] masahi commented on a change in pull request #7391: [ONNX] Add CumSum operator to ONNX frontend
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #7391:
URL: https://github.com/apache/tvm/pull/7391#discussion_r572706438
##########
File path: tests/python/frontend/onnx/test_forward.py
##########
@@ -3964,6 +3964,69 @@ def verify_softplus(indata):
verify_softplus(input_data)
+def test_cumsum():
+ def verify_cumsum(indata, axis, exclusive=0, reverse=0):
+ cumsum_node = onnx.helper.make_node(
+ "CumSum",
+ inputs=["X", "axis"],
+ outputs=["Y"],
+ )
+ if exclusive != 0:
+ exclusive_attr = helper.make_attribute("exclusive", exclusive)
+ cumsum_node.attribute.append(exclusive_attr)
+ if reverse != 0:
+ reverse_attr = helper.make_attribute("reverse", reverse)
+ cumsum_node.attribute.append(reverse_attr)
+ nodes = [
+ make_constant_node("axis", onnx.TensorProto.INT32, [1], [axis]),
+ cumsum_node,
+ ]
+
+ graph = helper.make_graph(
+ nodes,
+ "cumsum_test",
+ inputs=[
+ helper.make_tensor_value_info("X", TensorProto.FLOAT, list(indata.shape)),
Review comment:
Please add tests on an integer 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] mbrookhart commented on pull request #7391: [ONNX] Add CumSum operator to ONNX frontend
Posted by GitBox <gi...@apache.org>.
mbrookhart commented on pull request #7391:
URL: https://github.com/apache/tvm/pull/7391#issuecomment-776245481
Thanks @echuraev @masahi
----------------------------------------------------------------
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