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/06/19 15:55:47 UTC
[GitHub] [incubator-tvm] zhiics opened a new pull request #5855: [RELAY][VM] Add shape_of instruction
zhiics opened a new pull request #5855:
URL: https://github.com/apache/incubator-tvm/pull/5855
This PR adds a VM instruction `ShapeOf` to get the shape of a tensor at runtime. This instruction is always executed on CPU as there is no computation required.
cc @icemelon9 @jroesch @wweic
----------------------------------------------------------------
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] zhiics commented on a change in pull request #5855: [RELAY][VM] Add shape_of instruction
Posted by GitBox <gi...@apache.org>.
zhiics commented on a change in pull request #5855:
URL: https://github.com/apache/incubator-tvm/pull/5855#discussion_r444506282
##########
File path: python/tvm/relay/op/memory/memory.py
##########
@@ -104,6 +104,21 @@ def shape_func(func, inputs, outputs, dependent=False):
"""
return _make.shape_func(func, inputs, outputs, dependent)
+def shape_of(expr):
Review comment:
yeah, maybe `invoke_tvm_op` should be ported there as well later?
----------------------------------------------------------------
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] zhiics commented on a change in pull request #5855: [RELAY][VM] Add shape_of instruction
Posted by GitBox <gi...@apache.org>.
zhiics commented on a change in pull request #5855:
URL: https://github.com/apache/incubator-tvm/pull/5855#discussion_r444325734
##########
File path: src/relay/op/memory/memory.cc
##########
@@ -423,5 +427,24 @@ RELAY_REGISTER_OP("memory.shape_func")
return {topi::identity(inputs[0])};
});
+RELAY_REGISTER_OP("memory.shape_of")
Review comment:
yeah, make sense. We probably also want to refactor other instructions.
----------------------------------------------------------------
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] zhiics commented on a change in pull request #5855: [RELAY][VM] Add shape_of instruction
Posted by GitBox <gi...@apache.org>.
zhiics commented on a change in pull request #5855:
URL: https://github.com/apache/incubator-tvm/pull/5855#discussion_r446250485
##########
File path: python/tvm/relay/op/dialect/vm.py
##########
@@ -0,0 +1,35 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
Review comment:
Thanks for pointing out. I changed it to relay.op.vm. Maybe we should have such a namespace in the long run? Likely, we can have dialect.vm, dialect.memory, dialect.qnn, etc.
----------------------------------------------------------------
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] zhiics commented on a change in pull request #5855: [RELAY][VM] Add shape_of instruction
Posted by GitBox <gi...@apache.org>.
zhiics commented on a change in pull request #5855:
URL: https://github.com/apache/incubator-tvm/pull/5855#discussion_r444493947
##########
File path: python/tvm/relay/op/memory/memory.py
##########
@@ -104,6 +104,21 @@ def shape_func(func, inputs, outputs, dependent=False):
"""
return _make.shape_func(func, inputs, outputs, dependent)
+def shape_of(expr):
Review comment:
Yeah, I agree with you. The reason why I left it here was because I think we may need to move all these ops under op/vm. How do you think?
----------------------------------------------------------------
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] icemelon9 commented on a change in pull request #5855: [RELAY][VM] Add shape_of instruction
Posted by GitBox <gi...@apache.org>.
icemelon9 commented on a change in pull request #5855:
URL: https://github.com/apache/incubator-tvm/pull/5855#discussion_r444000215
##########
File path: src/relay/op/memory/memory.cc
##########
@@ -423,5 +427,24 @@ RELAY_REGISTER_OP("memory.shape_func")
return {topi::identity(inputs[0])};
});
+RELAY_REGISTER_OP("memory.shape_of")
Review comment:
I think we should call it `vm.shape_of` instead of `memory.shape_of`.
----------------------------------------------------------------
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] jroesch commented on pull request #5855: [RELAY][VM] Add shape_of instruction
Posted by GitBox <gi...@apache.org>.
jroesch commented on pull request #5855:
URL: https://github.com/apache/incubator-tvm/pull/5855#issuecomment-647845719
cc @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] icemelon9 commented on a change in pull request #5855: [RELAY][VM] Add shape_of instruction
Posted by GitBox <gi...@apache.org>.
icemelon9 commented on a change in pull request #5855:
URL: https://github.com/apache/incubator-tvm/pull/5855#discussion_r446347040
##########
File path: src/relay/op/vm/vm.cc
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/*!
+ * \file src/relay/op/vm/vm.cc
+ * \brief Dialect operators for Relay VM.
+ */
+
+#include <topi/elemwise.h>
+#include <tvm/relay/expr.h>
+#include <tvm/relay/op.h>
+#include <tvm/relay/op_attr_types.h>
+#include <tvm/runtime/data_type.h>
+
+#include "../../transforms/infer_layout_util.h"
+#include "../op_common.h"
+#include "../type_relations.h"
+
+namespace tvm {
+namespace relay {
+
+// Forward declare the shape_of type relation function.
+bool ShapeOfRel(const Array<Type>& types, int num_inputs, const Attrs& attrs,
Review comment:
We could just put `ShapeOfRel` into header file.
----------------------------------------------------------------
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] zhiics commented on a change in pull request #5855: [RELAY][VM] Add shape_of instruction
Posted by GitBox <gi...@apache.org>.
zhiics commented on a change in pull request #5855:
URL: https://github.com/apache/incubator-tvm/pull/5855#discussion_r446401587
##########
File path: src/relay/op/vm/vm.cc
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/*!
+ * \file src/relay/op/vm/vm.cc
+ * \brief Dialect operators for Relay VM.
+ */
+
+#include <topi/elemwise.h>
+#include <tvm/relay/expr.h>
+#include <tvm/relay/op.h>
+#include <tvm/relay/op_attr_types.h>
+#include <tvm/runtime/data_type.h>
+
+#include "../../transforms/infer_layout_util.h"
+#include "../op_common.h"
+#include "../type_relations.h"
+
+namespace tvm {
+namespace relay {
+
+// Forward declare the shape_of type relation function.
+bool ShapeOfRel(const Array<Type>& types, int num_inputs, const Attrs& attrs,
Review comment:
Yeah, I was a bit hesitating in doing this as there are only two uses. I will move the declaration to type_relations.h since you also have this consideration.
----------------------------------------------------------------
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 #5855: [RELAY][VM] Add shape_of instruction
Posted by GitBox <gi...@apache.org>.
mbrookhart commented on a change in pull request #5855:
URL: https://github.com/apache/incubator-tvm/pull/5855#discussion_r444338874
##########
File path: tests/python/relay/test_vm_serialization.py
##########
@@ -332,6 +284,20 @@ def test_mobilenet():
run_network(mod, params)
+def test_vm_shape_of():
Review comment:
I don't understand how this test hits the shape_of command? Are we sure this is testing the new 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] [incubator-tvm] icemelon9 commented on a change in pull request #5855: [RELAY][VM] Add shape_of instruction
Posted by GitBox <gi...@apache.org>.
icemelon9 commented on a change in pull request #5855:
URL: https://github.com/apache/incubator-tvm/pull/5855#discussion_r444501324
##########
File path: python/tvm/relay/op/memory/memory.py
##########
@@ -104,6 +104,21 @@ def shape_func(func, inputs, outputs, dependent=False):
"""
return _make.shape_func(func, inputs, outputs, dependent)
+def shape_of(expr):
Review comment:
I think memory related ops should be still in the `memory` namespace such that potentially other runtime can reuse 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] [incubator-tvm] zhiics commented on a change in pull request #5855: [RELAY][VM] Add shape_of instruction
Posted by GitBox <gi...@apache.org>.
zhiics commented on a change in pull request #5855:
URL: https://github.com/apache/incubator-tvm/pull/5855#discussion_r444355425
##########
File path: tests/python/relay/test_vm_serialization.py
##########
@@ -332,6 +284,20 @@ def test_mobilenet():
run_network(mod, params)
+def test_vm_shape_of():
Review comment:
`vm.shape_of` is implicitly added by memory manifest pass when a dynamic input is detected. It is used to get the shape of a tensor at runtime and it has to be on the CPU as the following instructions (i.e. `shape_func`) would be on CPU as well. The normal `shape_of` instruction doesn't have this constraint so we don't actually convert them.
https://github.com/apache/incubator-tvm/blob/ef9bf7d957665901c6980b9ada2aeb8e38d435c6/python/tvm/relay/transform/memory_alloc.py#L141
----------------------------------------------------------------
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 #5855: [RELAY][VM] Add shape_of instruction
Posted by GitBox <gi...@apache.org>.
mbrookhart commented on a change in pull request #5855:
URL: https://github.com/apache/incubator-tvm/pull/5855#discussion_r444338874
##########
File path: tests/python/relay/test_vm_serialization.py
##########
@@ -332,6 +284,20 @@ def test_mobilenet():
run_network(mod, params)
+def test_vm_shape_of():
Review comment:
I don't understand how this test hits the shape_of command?
----------------------------------------------------------------
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] tqchen merged pull request #5855: [RELAY][VM] Add shape_of instruction
Posted by GitBox <gi...@apache.org>.
tqchen merged pull request #5855:
URL: https://github.com/apache/incubator-tvm/pull/5855
----------------------------------------------------------------
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] icemelon9 commented on pull request #5855: [RELAY][VM] Add shape_of instruction
Posted by GitBox <gi...@apache.org>.
icemelon9 commented on pull request #5855:
URL: https://github.com/apache/incubator-tvm/pull/5855#issuecomment-648395868
I feel we should treat ops like `vm.shape_of` as a dialect for vm. Therefore the conversion from relay ops to vm dialect ops should not be inside the manifest memory pass, but a separate pass.
@jroesch @tqchen What's your thoughts on this?
----------------------------------------------------------------
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] tqchen commented on a change in pull request #5855: [RELAY][VM] Add shape_of instruction
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #5855:
URL: https://github.com/apache/incubator-tvm/pull/5855#discussion_r446214527
##########
File path: python/tvm/relay/op/dialect/vm.py
##########
@@ -0,0 +1,35 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
Review comment:
Let us simply use relay.op.vm and mark it as a dialect in the comment. Since QNN is also a dialect but does not belong to the dialect namespace
##########
File path: python/tvm/relay/op/dialect/_make.py
##########
@@ -0,0 +1,20 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Constructor APIs"""
+import tvm._ffi
+
+tvm._ffi._init_api("relay.op.dialect._make", __name__)
Review comment:
Use the new style constructor _ffi_api.py as in other parts
----------------------------------------------------------------
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] icemelon9 commented on a change in pull request #5855: [RELAY][VM] Add shape_of instruction
Posted by GitBox <gi...@apache.org>.
icemelon9 commented on a change in pull request #5855:
URL: https://github.com/apache/incubator-tvm/pull/5855#discussion_r444480395
##########
File path: python/tvm/relay/op/memory/memory.py
##########
@@ -104,6 +104,21 @@ def shape_func(func, inputs, outputs, dependent=False):
"""
return _make.shape_func(func, inputs, outputs, dependent)
+def shape_of(expr):
Review comment:
since we already call it `vm.shape_of` in the cpp, I think we should also put `shape_of` into the folder of op/vm.
----------------------------------------------------------------
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] zhiics commented on pull request #5855: [RELAY][VM] Add shape_of instruction
Posted by GitBox <gi...@apache.org>.
zhiics commented on pull request #5855:
URL: https://github.com/apache/incubator-tvm/pull/5855#issuecomment-650654229
@tqchen could you please take another look?
----------------------------------------------------------------
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 #5855: [RELAY][VM] Add shape_of instruction
Posted by GitBox <gi...@apache.org>.
mbrookhart commented on a change in pull request #5855:
URL: https://github.com/apache/incubator-tvm/pull/5855#discussion_r444426108
##########
File path: tests/python/relay/test_vm_serialization.py
##########
@@ -332,6 +284,20 @@ def test_mobilenet():
run_network(mod, params)
+def test_vm_shape_of():
Review comment:
:+1: Got it, 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