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 2020/05/18 22:43:29 UTC
[GitHub] [incubator-tvm] mbrookhart opened a new pull request #5619: Add Scatter to Topi/Relay/ONNX via hybrid script
mbrookhart opened a new pull request #5619:
URL: https://github.com/apache/incubator-tvm/pull/5619
cc: @jwfromm @tqchen @masahi
The actual hybrid script implementation is kind of gross, I can't seem to figure out a way to clean it up without causing very opaque errors in the hybrid script module. If anyone has suggestions on cleaning that up, I'd love to hear them.
----------------------------------------------------------------
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] [incubator-tvm] kevinthesun commented on a change in pull request #5619: Add Scatter to Topi/Relay/ONNX via hybrid script
Posted by GitBox <gi...@apache.org>.
kevinthesun commented on a change in pull request #5619:
URL: https://github.com/apache/incubator-tvm/pull/5619#discussion_r434057734
##########
File path: python/tvm/relay/op/_transform.py
##########
@@ -383,6 +391,13 @@ def argwhere_shape_func(attrs, inputs, out_ndims):
return [_argwhere_shape_func_5d(inputs[0])]
return ValueError("Does not support rank higher than 5 in argwhere")
+@_reg.register_shape_func("scatter", True)
Review comment:
Directly use elemwise shape func: ```register_shape_func("scatter", False, elemwise_shape_func)```
----------------------------------------------------------------
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] [incubator-tvm] notoraptor commented on pull request #5619: Add Scatter to Topi/Relay/ONNX via hybrid script
Posted by GitBox <gi...@apache.org>.
notoraptor commented on pull request #5619:
URL: https://github.com/apache/incubator-tvm/pull/5619#issuecomment-640834212
Hi @mbrookhart ! Is this implementation of `scatter` the same as the one in pytorch here ? https://pytorch.org/docs/stable/tensors.html#torch.Tensor.scatter_
As I implemented pytorch's `gather` in #5716 , I planned to implement reverse operation `scatter` too.
----------------------------------------------------------------
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] [incubator-tvm] mbrookhart commented on pull request #5619: Add Scatter to Topi/Relay/ONNX via hybrid script
Posted by GitBox <gi...@apache.org>.
mbrookhart commented on pull request #5619:
URL: https://github.com/apache/incubator-tvm/pull/5619#issuecomment-637732830
@jwfromm @masahi @Laurawly @Huyuwei I finally found time to get back to this and fix the issues with GPU/i386. Could you take a look? Thanks!
----------------------------------------------------------------
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] [incubator-tvm] mbrookhart commented on a change in pull request #5619: Add Scatter to Topi/Relay/ONNX via hybrid script
Posted by GitBox <gi...@apache.org>.
mbrookhart commented on a change in pull request #5619:
URL: https://github.com/apache/incubator-tvm/pull/5619#discussion_r437004308
##########
File path: src/relay/op/tensor/transform.cc
##########
@@ -781,6 +781,53 @@ non-zero)doc" TVM_ADD_FILELINE)
.set_attr<TOpPattern>("TOpPattern", kOpaque)
.set_support_level(10);
+// Scatter
+TVM_REGISTER_NODE_TYPE(ScatterAttrs);
+
+// Scatter
+bool ScatterRel(const Array<Type>& types, int num_inputs, const Attrs& attrs,
+ const TypeReporter& reporter) {
+ CHECK_EQ(num_inputs, 3);
+ CHECK_EQ(types.size(), 4);
+ auto data = types[0].as<TensorTypeNode>();
+ if (data == nullptr) {
+ return false;
+ }
+ auto indices = types[1].as<TensorTypeNode>();
+ if (indices == nullptr) {
+ return false;
+ }
+ auto updates = types[2].as<TensorTypeNode>();
+ if (updates == nullptr) {
+ return false;
+ }
+ CHECK(indices->dtype.is_int()) << "indices of take must be tensor of integer";
+ const auto param = attrs.as<ScatterAttrs>();
+ CHECK(param != nullptr);
+ reporter->Assign(types[3], TensorType(data->shape, data->dtype));
+ return true;
+}
+
+TVM_REGISTER_GLOBAL("relay.op._make.scatter")
+ .set_body_typed([](Expr data, Expr indices, Expr updates, int axis) {
+ auto attrs = make_object<ScatterAttrs>();
+ attrs->axis = std::move(axis);
+ static const Op& op = Op::Get("scatter");
+ return Call(op, {data, indices, updates}, Attrs(attrs), {});
+ });
+
+RELAY_REGISTER_OP("scatter")
+ .describe(
+ R"doc(Update data at positions defined by indices with values in updates)doc" TVM_ADD_FILELINE)
+ .set_num_inputs(3)
+ .add_argument("data", "Tensor", "The input data tensor.")
+ .add_argument("indicies", "Tensor", "The indicies location tensor.")
+ .add_argument("updates", "Tensor", "The values to update the input with.")
+ .add_type_rel("Scatter", ScatterRel)
+ .set_attr<TOpIsStateful>("TOpIsStateful", false)
+ .set_attr<TOpPattern>("TOpPattern", kOpaque)
Review comment:
Because I washing sure and I wanted to be conservative :) It might be Injective.
##########
File path: src/relay/op/tensor/transform.cc
##########
@@ -781,6 +781,53 @@ non-zero)doc" TVM_ADD_FILELINE)
.set_attr<TOpPattern>("TOpPattern", kOpaque)
.set_support_level(10);
+// Scatter
+TVM_REGISTER_NODE_TYPE(ScatterAttrs);
+
+// Scatter
+bool ScatterRel(const Array<Type>& types, int num_inputs, const Attrs& attrs,
+ const TypeReporter& reporter) {
+ CHECK_EQ(num_inputs, 3);
+ CHECK_EQ(types.size(), 4);
+ auto data = types[0].as<TensorTypeNode>();
+ if (data == nullptr) {
+ return false;
+ }
+ auto indices = types[1].as<TensorTypeNode>();
+ if (indices == nullptr) {
+ return false;
+ }
+ auto updates = types[2].as<TensorTypeNode>();
+ if (updates == nullptr) {
+ return false;
+ }
+ CHECK(indices->dtype.is_int()) << "indices of take must be tensor of integer";
+ const auto param = attrs.as<ScatterAttrs>();
+ CHECK(param != nullptr);
+ reporter->Assign(types[3], TensorType(data->shape, data->dtype));
+ return true;
+}
+
+TVM_REGISTER_GLOBAL("relay.op._make.scatter")
+ .set_body_typed([](Expr data, Expr indices, Expr updates, int axis) {
+ auto attrs = make_object<ScatterAttrs>();
+ attrs->axis = std::move(axis);
+ static const Op& op = Op::Get("scatter");
+ return Call(op, {data, indices, updates}, Attrs(attrs), {});
+ });
+
+RELAY_REGISTER_OP("scatter")
+ .describe(
+ R"doc(Update data at positions defined by indices with values in updates)doc" TVM_ADD_FILELINE)
+ .set_num_inputs(3)
+ .add_argument("data", "Tensor", "The input data tensor.")
+ .add_argument("indicies", "Tensor", "The indicies location tensor.")
+ .add_argument("updates", "Tensor", "The values to update the input with.")
+ .add_type_rel("Scatter", ScatterRel)
+ .set_attr<TOpIsStateful>("TOpIsStateful", false)
+ .set_attr<TOpPattern>("TOpPattern", kOpaque)
Review comment:
Because I wasn't sure and I wanted to be conservative :) It might be Injective.
----------------------------------------------------------------
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] [incubator-tvm] masahi commented on pull request #5619: Add Scatter to Topi/Relay/ONNX via hybrid script
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #5619:
URL: https://github.com/apache/incubator-tvm/pull/5619#issuecomment-641616394
Thanks @mbrookhart
----------------------------------------------------------------
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] [incubator-tvm] masahi merged pull request #5619: Add Scatter to Topi/Relay/ONNX via hybrid script
Posted by GitBox <gi...@apache.org>.
masahi merged pull request #5619:
URL: https://github.com/apache/incubator-tvm/pull/5619
----------------------------------------------------------------
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] [incubator-tvm] masahi commented on a change in pull request #5619: Add Scatter to Topi/Relay/ONNX via hybrid script
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #5619:
URL: https://github.com/apache/incubator-tvm/pull/5619#discussion_r437014125
##########
File path: src/relay/op/tensor/transform.cc
##########
@@ -781,6 +781,53 @@ non-zero)doc" TVM_ADD_FILELINE)
.set_attr<TOpPattern>("TOpPattern", kOpaque)
.set_support_level(10);
+// Scatter
+TVM_REGISTER_NODE_TYPE(ScatterAttrs);
+
+// Scatter
+bool ScatterRel(const Array<Type>& types, int num_inputs, const Attrs& attrs,
+ const TypeReporter& reporter) {
+ CHECK_EQ(num_inputs, 3);
+ CHECK_EQ(types.size(), 4);
+ auto data = types[0].as<TensorTypeNode>();
+ if (data == nullptr) {
+ return false;
+ }
+ auto indices = types[1].as<TensorTypeNode>();
+ if (indices == nullptr) {
+ return false;
+ }
+ auto updates = types[2].as<TensorTypeNode>();
+ if (updates == nullptr) {
+ return false;
+ }
+ CHECK(indices->dtype.is_int()) << "indices of take must be tensor of integer";
+ const auto param = attrs.as<ScatterAttrs>();
+ CHECK(param != nullptr);
+ reporter->Assign(types[3], TensorType(data->shape, data->dtype));
+ return true;
+}
+
+TVM_REGISTER_GLOBAL("relay.op._make.scatter")
+ .set_body_typed([](Expr data, Expr indices, Expr updates, int axis) {
+ auto attrs = make_object<ScatterAttrs>();
+ attrs->axis = std::move(axis);
+ static const Op& op = Op::Get("scatter");
+ return Call(op, {data, indices, updates}, Attrs(attrs), {});
+ });
+
+RELAY_REGISTER_OP("scatter")
+ .describe(
+ R"doc(Update data at positions defined by indices with values in updates)doc" TVM_ADD_FILELINE)
+ .set_num_inputs(3)
+ .add_argument("data", "Tensor", "The input data tensor.")
+ .add_argument("indicies", "Tensor", "The indicies location tensor.")
+ .add_argument("updates", "Tensor", "The values to update the input with.")
+ .add_type_rel("Scatter", ScatterRel)
+ .set_attr<TOpIsStateful>("TOpIsStateful", false)
+ .set_attr<TOpPattern>("TOpPattern", kOpaque)
Review comment:
ok for 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] masahi commented on a change in pull request #5619: Add Scatter to Topi/Relay/ONNX via hybrid script
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #5619:
URL: https://github.com/apache/incubator-tvm/pull/5619#discussion_r435627742
##########
File path: src/relay/op/tensor/transform.cc
##########
@@ -781,6 +781,53 @@ non-zero)doc" TVM_ADD_FILELINE)
.set_attr<TOpPattern>("TOpPattern", kOpaque)
.set_support_level(10);
+// Scatter
+TVM_REGISTER_NODE_TYPE(ScatterAttrs);
+
+// Scatter
+bool ScatterRel(const Array<Type>& types, int num_inputs, const Attrs& attrs,
+ const TypeReporter& reporter) {
+ CHECK_EQ(num_inputs, 3);
+ CHECK_EQ(types.size(), 4);
+ auto data = types[0].as<TensorTypeNode>();
+ if (data == nullptr) {
+ return false;
+ }
+ auto indices = types[1].as<TensorTypeNode>();
+ if (indices == nullptr) {
+ return false;
+ }
+ auto updates = types[2].as<TensorTypeNode>();
+ if (updates == nullptr) {
+ return false;
+ }
+ CHECK(indices->dtype.is_int()) << "indices of take must be tensor of integer";
+ const auto param = attrs.as<ScatterAttrs>();
+ CHECK(param != nullptr);
+ reporter->Assign(types[3], TensorType(data->shape, data->dtype));
+ return true;
+}
+
+TVM_REGISTER_GLOBAL("relay.op._make.scatter")
+ .set_body_typed([](Expr data, Expr indices, Expr updates, int axis) {
+ auto attrs = make_object<ScatterAttrs>();
+ attrs->axis = std::move(axis);
+ static const Op& op = Op::Get("scatter");
+ return Call(op, {data, indices, updates}, Attrs(attrs), {});
+ });
+
+RELAY_REGISTER_OP("scatter")
+ .describe(
+ R"doc(Update data at positions defined by indices with values in updates)doc" TVM_ADD_FILELINE)
+ .set_num_inputs(3)
+ .add_argument("data", "Tensor", "The input data tensor.")
+ .add_argument("indicies", "Tensor", "The indicies location tensor.")
+ .add_argument("updates", "Tensor", "The values to update the input with.")
+ .add_type_rel("Scatter", ScatterRel)
+ .set_attr<TOpIsStateful>("TOpIsStateful", false)
+ .set_attr<TOpPattern>("TOpPattern", kOpaque)
Review comment:
Why opaque?
----------------------------------------------------------------
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] [incubator-tvm] mbrookhart commented on pull request #5619: Add Scatter to Topi/Relay/ONNX via hybrid script
Posted by GitBox <gi...@apache.org>.
mbrookhart commented on pull request #5619:
URL: https://github.com/apache/incubator-tvm/pull/5619#issuecomment-640890148
@notoraptor I based it off ONNX, which seems to be doing the same thing as the Pytorch op, if I'm reading the docs correctly. So, I think so, yes.
----------------------------------------------------------------
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] [incubator-tvm] masahi commented on pull request #5619: Add Scatter to Topi/Relay/ONNX via hybrid script
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #5619:
URL: https://github.com/apache/incubator-tvm/pull/5619#issuecomment-640940800
@mbrookhart There is some issue with WASM build, can you try again?
cc @tqchen https://ci.tvm.ai/blue/organizations/jenkins/tvm/detail/PR-5619/9/pipeline
----------------------------------------------------------------
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