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/11/10 02:00:00 UTC
[GitHub] [incubator-tvm] lixiaoquan opened a new pull request #6890: [Relay] Fix a bug in tensor_array_scatter
lixiaoquan opened a new pull request #6890:
URL: https://github.com/apache/incubator-tvm/pull/6890
tensor_array_scatter constructs helper functions according to dtype
and shape of element. When there are multiple scatter operations with
same dtype and element shape but different indicies_shape, there will
be name conflict in prelude.
----------------------------------------------------------------
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] lixiaoquan commented on a change in pull request #6890: [Relay] Fix a bug in tensor_array_scatter
Posted by GitBox <gi...@apache.org>.
lixiaoquan commented on a change in pull request #6890:
URL: https://github.com/apache/incubator-tvm/pull/6890#discussion_r526577147
##########
File path: python/tvm/relay/prelude.py
##########
@@ -383,6 +383,12 @@ def define_tensor_array_scatter(self, indices_shape=None, force_update=False):
return
tensor_array_scatter_helper_name = self.get_name("tensor_array_scatter_helper")
+
+ if indices_shape:
Review comment:
Please see this line
https://github.com/apache/incubator-tvm/blob/7649075fbb71ecab0a41c6fe4d41a86724e42e7a/python/tvm/relay/prelude.py#L425
force_update works, and scatter funciton in mod is replaced, but the original one for first scatter will be needed too.
Consider the case I added, there are two scatters in this case. If we don't put indices into name, tensor_array_scatter_var will be the same for two scatters, but Function will be different because of different indices. The first function is overrided by the second one, and the second Function can match the first scatter's type.
----------------------------------------------------------------
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 pull request #6890: [Relay] Fix a bug in tensor_array_scatter
Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #6890:
URL: https://github.com/apache/incubator-tvm/pull/6890#issuecomment-726415566
cc @mbrookhart @jroesch
----------------------------------------------------------------
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] lixiaoquan commented on a change in pull request #6890: [Relay] Fix a bug in tensor_array_scatter
Posted by GitBox <gi...@apache.org>.
lixiaoquan commented on a change in pull request #6890:
URL: https://github.com/apache/incubator-tvm/pull/6890#discussion_r526577147
##########
File path: python/tvm/relay/prelude.py
##########
@@ -383,6 +383,12 @@ def define_tensor_array_scatter(self, indices_shape=None, force_update=False):
return
tensor_array_scatter_helper_name = self.get_name("tensor_array_scatter_helper")
+
+ if indices_shape:
Review comment:
Please see this line
https://github.com/apache/incubator-tvm/blob/7649075fbb71ecab0a41c6fe4d41a86724e42e7a/python/tvm/relay/prelude.py#L425
force_update works, and scatter funciton in mod is replaced, but the original one for first scatter will be needed too.
Consider the case I added, there are two scatters in this case. If we don't put indices into name, tensor_array_scatter_var will be the same for two scatters, but Function will be different because of different indices. The first function is overrided by the second one, and the second Function can't match the first scatter's type.
----------------------------------------------------------------
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 #6890: [Relay] Fix a bug in tensor_array_scatter
Posted by GitBox <gi...@apache.org>.
kevinthesun commented on a change in pull request #6890:
URL: https://github.com/apache/incubator-tvm/pull/6890#discussion_r526397043
##########
File path: python/tvm/relay/prelude.py
##########
@@ -383,6 +383,12 @@ def define_tensor_array_scatter(self, indices_shape=None, force_update=False):
return
tensor_array_scatter_helper_name = self.get_name("tensor_array_scatter_helper")
+
+ if indices_shape:
Review comment:
There is an ```force_update``` argument in tensor_array_scatter API. It would be better to add a test in test_adt.py and figure out why it is not updated in this case, rather than just modify the name.
Also tensor_array_split can have similar issue.
----------------------------------------------------------------
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 #6890: [Relay] Fix a bug in tensor_array_scatter
Posted by GitBox <gi...@apache.org>.
kevinthesun commented on a change in pull request #6890:
URL: https://github.com/apache/incubator-tvm/pull/6890#discussion_r528009277
##########
File path: python/tvm/relay/prelude.py
##########
@@ -383,6 +383,12 @@ def define_tensor_array_scatter(self, indices_shape=None, force_update=False):
return
tensor_array_scatter_helper_name = self.get_name("tensor_array_scatter_helper")
+
+ if indices_shape:
Review comment:
In this case, we might want to enhance ```get_name``` API to allow pass in extra argument shape info. Note that current tensor array op name is in the form of ```tensor_array_xxx_dtype_shape```. If multiple shapes are added, we need to add extra argument name, such as ```indice``` to make the name clear.
----------------------------------------------------------------
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] lixiaoquan commented on a change in pull request #6890: [Relay] Fix a bug in tensor_array_scatter
Posted by GitBox <gi...@apache.org>.
lixiaoquan commented on a change in pull request #6890:
URL: https://github.com/apache/incubator-tvm/pull/6890#discussion_r526577147
##########
File path: python/tvm/relay/prelude.py
##########
@@ -383,6 +383,12 @@ def define_tensor_array_scatter(self, indices_shape=None, force_update=False):
return
tensor_array_scatter_helper_name = self.get_name("tensor_array_scatter_helper")
+
+ if indices_shape:
Review comment:
Please see this line
https://github.com/apache/incubator-tvm/blob/7649075fbb71ecab0a41c6fe4d41a86724e42e7a/python/tvm/relay/prelude.py#L425
force_update works, and scatter funciton in mod is replaced, but the original one for first scatter will be needed too.
Consider the case I added, there are two scatters in this case. If we don't put indices into name, tensor_array_scatter_var will be the same for two scatters, but Function will be different. The first function is overrided by the second one, and the second Function can match the first scatter's type.
----------------------------------------------------------------
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 #6890: [Relay] Fix a bug in tensor_array_scatter
Posted by GitBox <gi...@apache.org>.
kevinthesun commented on a change in pull request #6890:
URL: https://github.com/apache/incubator-tvm/pull/6890#discussion_r528009277
##########
File path: python/tvm/relay/prelude.py
##########
@@ -383,6 +383,12 @@ def define_tensor_array_scatter(self, indices_shape=None, force_update=False):
return
tensor_array_scatter_helper_name = self.get_name("tensor_array_scatter_helper")
+
+ if indices_shape:
Review comment:
In this case, we might want to enhance ```get_name``` API to allow pass in extra argument shape info. Note that current tensor array op name is in the form of ```tensor_array_xxx_dtype_shape```. If multiple shapes are added, we need to add extra argument name, such as ```indices``` to make the name clear.
For example, previously we have ```tensor_array_scatter_float32_1_2_3```. We know the input ta dtype is float32 and tensor shape is (1, 2, 3). If we want to include indices shape, we might want to generate something like ```tensor_array_scatter_float32_1_2_3_indices_4```.
----------------------------------------------------------------
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