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/12/31 00:11:14 UTC
[GitHub] [tvm] gussmith23 opened a new pull request #9816: [WIP] Add windows operator
gussmith23 opened a new pull request #9816:
URL: https://github.com/apache/tvm/pull/9816
Adds the windows operator, which slides a window over a tensor to produce a new tensor of all possible windows. This is an essential operation for implementing the `im2col` transformation, which transforms a conv2d into a matrix multiplication by constructing the windows over the activation tensor and storing them explicitly in memory.
This is the first time I've added an operator to TVM, so please let me know if I've missed anything. I am also happy to spend more time adding more tests and documentation.
--
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] gussmith23 commented on pull request #9816: Add windows operator
Posted by GitBox <gi...@apache.org>.
gussmith23 commented on pull request #9816:
URL: https://github.com/apache/tvm/pull/9816#issuecomment-1004534948
Force-pushed to retrigger tests. Unsure why the test failed last time -- it failed on a seemingly unrelated test (an autotuner test).
--
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] gussmith23 commented on a change in pull request #9816: Add windows operator
Posted by GitBox <gi...@apache.org>.
gussmith23 commented on a change in pull request #9816:
URL: https://github.com/apache/tvm/pull/9816#discussion_r778344021
##########
File path: include/tvm/topi/transform.h
##########
@@ -47,6 +47,92 @@ namespace topi {
using namespace tvm::te;
using namespace topi::detail;
+/*!
+ * \brief Creates an operation to form windows over the input x.
+ *
+ * \param x The input tensor.
+ * \param axis What axis the windows begin forming over. Windows will be formed
+ * over this axis and all following axes. The axis value determines the window
+ * shape (and thus, the number of strides): window shape and strides must both
+ * be of length `data.ndim-axis`.
+ * \param window_shape The window shape to form over the input. Window shape
+ * must be of length `data.ndim-axis`.
+ * \param strides How to stride the window along each dimension. Strides must be
+ * of length `data.ndim-axis`.
+ * \param name The name of the operation
+ * \param tag The tag to mark the operation
+ *
+ * \return A Tensor whose op member is the windows operation
+ */
+inline Tensor windows(const Tensor& x, int axis, Array<Integer> window_shape,
+ Array<Integer> strides, std::string name = "T_windows",
+ // TODO(@gussmith23) what to tag it?
+ std::string tag = "") {
+ ICHECK(axis <= 0);
+ auto _axis = size_t(axis);
+ ICHECK(_axis < x->shape.size()) << "axis must be a valid dimension index of x.";
+ ICHECK(x->shape.size() - _axis == window_shape.size())
+ << "There must be a window shape for every dimension of x over which we are forming windows.";
+ ICHECK(strides.size() == window_shape.size()) << "Windows and strides should be the same length.";
Review comment:
Thanks, that makes sense after looking at the implementations of CHECK and ICHECK. I think some of these should still be ICHECKs (if ICHECK truly means "internal"). I will change some of them to CHECKs, but let me know if you think I should make them all CHECKs.
--
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] tkonolige commented on pull request #9816: Add windows operator
Posted by GitBox <gi...@apache.org>.
tkonolige commented on pull request #9816:
URL: https://github.com/apache/tvm/pull/9816#issuecomment-1005198398
I do think having an example is useful.
--
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 #9816: Add windows operator
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #9816:
URL: https://github.com/apache/tvm/pull/9816#issuecomment-1008446764
I don't like the name "windows". How about `sliding_window`?
--
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 #9816: Add sliding_window operator
Posted by GitBox <gi...@apache.org>.
masahi merged pull request #9816:
URL: https://github.com/apache/tvm/pull/9816
--
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] gussmith23 commented on pull request #9816: Add windows operator
Posted by GitBox <gi...@apache.org>.
gussmith23 commented on pull request #9816:
URL: https://github.com/apache/tvm/pull/9816#issuecomment-1005202174
I was getting errors - - is there documentation on how to write examples? all the other examples in this file are rather simple, but to illustrate the use I think we need to use at least a relay executor and numpy random tensors. is that possible?
--
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] gussmith23 commented on pull request #9816: Add windows operator
Posted by GitBox <gi...@apache.org>.
gussmith23 commented on pull request #9816:
URL: https://github.com/apache/tvm/pull/9816#issuecomment-1009297497
@masahi done!
--
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] gussmith23 commented on pull request #9816: Add windows operator
Posted by GitBox <gi...@apache.org>.
gussmith23 commented on pull request #9816:
URL: https://github.com/apache/tvm/pull/9816#issuecomment-1006872898
Bump, ready for review by a code owner.
--
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] gussmith23 removed a comment on pull request #9816: Add windows operator
Posted by GitBox <gi...@apache.org>.
gussmith23 removed a comment on pull request #9816:
URL: https://github.com/apache/tvm/pull/9816#issuecomment-1004534464
Unsure why this test failed -- is it possibly a flaky test? Can someone re-run the test for me?
--
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] gussmith23 commented on a change in pull request #9816: Add windows operator
Posted by GitBox <gi...@apache.org>.
gussmith23 commented on a change in pull request #9816:
URL: https://github.com/apache/tvm/pull/9816#discussion_r778350526
##########
File path: src/relay/op/tensor/transform.cc
##########
@@ -51,6 +51,76 @@ namespace tvm {
namespace relay {
using tir::IntImmNode;
+TVM_REGISTER_NODE_TYPE(WindowsAttrs);
+
+bool WindowsRel(const Array<Type>& types, int num_inputs, const Attrs& attrs,
+ const TypeReporter& reporter) {
+ // `types` contains: [data, result]
+ ICHECK_EQ(types.size(), 2);
+ const auto* data = types[0].as<TensorTypeNode>();
+ if (data == nullptr) {
+ ICHECK(types[0].as<IncompleteTypeNode>())
+ << "windows: expect input type to be TensorType but get " << types[0];
Review comment:
Thanks, done!
--
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] gussmith23 commented on pull request #9816: Add windows operator
Posted by GitBox <gi...@apache.org>.
gussmith23 commented on pull request #9816:
URL: https://github.com/apache/tvm/pull/9816#issuecomment-1005937679
Oh, huh, i was getting this error:
```
/workspace/python/tvm/relay/__init__.py:docstring of tvm.relay.op.transform.windows:22: WARNING: Error in "code-block" directive:
maximum 1 argument(s) allowed, 51 supplied.
.. code-block:: python
# Slide a window of shape (3, 4, 5) over the x tensor, beginning with
<the rest of the example code>
```
I assumed this was because something was trying to run the Python code, which is why I removed the example. Turns out, I think it's because there was no newline after the `.. code-block` directive! The error message from Sphinx was a bit unintuitive. Let's see if adding a newline fixes things.
--
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] gussmith23 commented on pull request #9816: Add windows operator
Posted by GitBox <gi...@apache.org>.
gussmith23 commented on pull request #9816:
URL: https://github.com/apache/tvm/pull/9816#issuecomment-1004534464
Unsure why this test failed -- is it possibly a flaky test? Can someone re-run the test for me?
--
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] tkonolige commented on a change in pull request #9816: Add windows operator
Posted by GitBox <gi...@apache.org>.
tkonolige commented on a change in pull request #9816:
URL: https://github.com/apache/tvm/pull/9816#discussion_r778260140
##########
File path: src/relay/op/tensor/transform.cc
##########
@@ -51,6 +51,76 @@ namespace tvm {
namespace relay {
using tir::IntImmNode;
+TVM_REGISTER_NODE_TYPE(WindowsAttrs);
+
+bool WindowsRel(const Array<Type>& types, int num_inputs, const Attrs& attrs,
+ const TypeReporter& reporter) {
+ // `types` contains: [data, result]
+ ICHECK_EQ(types.size(), 2);
+ const auto* data = types[0].as<TensorTypeNode>();
+ if (data == nullptr) {
+ ICHECK(types[0].as<IncompleteTypeNode>())
+ << "windows: expect input type to be TensorType but get " << types[0];
Review comment:
It would be better to use the diagnostic context to report this error. You can find an example here: https://github.com/apache/tvm/blob/main/src/relay/op/nn/nn.cc#L65-L68.
##########
File path: include/tvm/topi/transform.h
##########
@@ -47,6 +47,92 @@ namespace topi {
using namespace tvm::te;
using namespace topi::detail;
+/*!
+ * \brief Creates an operation to form windows over the input x.
+ *
+ * \param x The input tensor.
+ * \param axis What axis the windows begin forming over. Windows will be formed
+ * over this axis and all following axes. The axis value determines the window
+ * shape (and thus, the number of strides): window shape and strides must both
+ * be of length `data.ndim-axis`.
+ * \param window_shape The window shape to form over the input. Window shape
+ * must be of length `data.ndim-axis`.
+ * \param strides How to stride the window along each dimension. Strides must be
+ * of length `data.ndim-axis`.
+ * \param name The name of the operation
+ * \param tag The tag to mark the operation
+ *
+ * \return A Tensor whose op member is the windows operation
+ */
+inline Tensor windows(const Tensor& x, int axis, Array<Integer> window_shape,
+ Array<Integer> strides, std::string name = "T_windows",
+ // TODO(@gussmith23) what to tag it?
+ std::string tag = "") {
+ ICHECK(axis <= 0);
+ auto _axis = size_t(axis);
+ ICHECK(_axis < x->shape.size()) << "axis must be a valid dimension index of x.";
+ ICHECK(x->shape.size() - _axis == window_shape.size())
+ << "There must be a window shape for every dimension of x over which we are forming windows.";
+ ICHECK(strides.size() == window_shape.size()) << "Windows and strides should be the same length.";
Review comment:
I think these should all be `CHCEK` instead of `ICHECK`. In general we use `CHECK` for things that could be caused by user error (though our definition of user error is very loose).
##########
File path: include/tvm/topi/transform.h
##########
@@ -47,6 +47,92 @@ namespace topi {
using namespace tvm::te;
using namespace topi::detail;
+/*!
+ * \brief Creates an operation to form windows over the input x.
+ *
+ * \param x The input tensor.
+ * \param axis What axis the windows begin forming over. Windows will be formed
+ * over this axis and all following axes. The axis value determines the window
+ * shape (and thus, the number of strides): window shape and strides must both
+ * be of length `data.ndim-axis`.
+ * \param window_shape The window shape to form over the input. Window shape
+ * must be of length `data.ndim-axis`.
+ * \param strides How to stride the window along each dimension. Strides must be
+ * of length `data.ndim-axis`.
+ * \param name The name of the operation
+ * \param tag The tag to mark the operation
+ *
+ * \return A Tensor whose op member is the windows operation
+ */
+inline Tensor windows(const Tensor& x, int axis, Array<Integer> window_shape,
+ Array<Integer> strides, std::string name = "T_windows",
+ // TODO(@gussmith23) what to tag it?
Review comment:
I think as you have it is fine.
--
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] tkonolige commented on pull request #9816: Add windows operator
Posted by GitBox <gi...@apache.org>.
tkonolige commented on pull request #9816:
URL: https://github.com/apache/tvm/pull/9816#issuecomment-1005919250
Here's an example of using an example: https://github.com/apache/tvm/blob/main/python/tvm/relay/op/random/kernel.py#L59-L67. Examples are not run, so they don't necessarily need to be valid 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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org