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/01/14 02:15:51 UTC
[GitHub] [incubator-tvm] huajsj opened a new pull request #4703: [VTA]
Support network which have no unique operator as start/stop name for graph
pack.
huajsj opened a new pull request #4703: [VTA] Support network which have no unique operator as start/stop name for graph pack.
URL: https://github.com/apache/incubator-tvm/pull/4703
[Issue]
Current vta use 'start' and 'stop' name to define the pack start point
and end point, but this method not work for these network which have
no 2 unique operator as start point and stop point.
[Solution]
In this solution we give 2 addtional parameters start_name_indx and
stop_name_indx to make vta pack logic work with the said network,
for exampl for following networks which have no unique operator,
%0 = nn.add
%1 = nn.conv2d
%2 = nn.batch_norm
%3 = nn.leaky_relu
%4 = nn.add
%5 = nn.conv2d
%6 = nn.batch_norm
%7 = nn.leaky_relu
%8 = nn.add
with this solution we can use following parameter format to make
vta work on it.
relay_prog = graph_pack(
//....
start_name="nn.add",
stop_name="nn.add",
start_name_indx=0,
stop_name_indx=2)
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] huajsj edited a comment on issue #4703: [VTA]
Support network which have no unique operator as start/stop name for graph
pack.
Posted by GitBox <gi...@apache.org>.
huajsj edited a comment on issue #4703: [VTA] Support network which have no unique operator as start/stop name for graph pack.
URL: https://github.com/apache/incubator-tvm/pull/4703#issuecomment-574325187
Hi Liangfu, Thanks for the comments, the reason here to use operator name and indices is to first compatible with existing implementation like resnet18 example, secondly try to make the logic to
be more readable for that from my point operator name is more friendly than indices only. please kindly let me know how you think.
Regards
Hua
> This is a favorable feature, since there could be more than one operator with the given name.
>
> I wonder whether we can bring the unique name of the layer, instead of the name of the operator, to define the subgraph. I think using indices of the named operators are less intuitive, comparing to unique names of the layers.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tmoreau89 commented on issue #4703: [VTA] Support
network which have no unique operator as start/stop name for graph pack.
Posted by GitBox <gi...@apache.org>.
tmoreau89 commented on issue #4703: [VTA] Support network which have no unique operator as start/stop name for graph pack.
URL: https://github.com/apache/incubator-tvm/pull/4703#issuecomment-574923019
Thanks for this useful feature @huajsj . I think that in the long terms this pass will need to be re-written; but for now this will fix the gap.
One thing I'd like to add is the possibility of passing the `-1` index to refer to the last occurrence of the operator in the network. Would that be possible to do?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] liangfu commented on issue #4703: [VTA] Support
network which have no unique operator as start/stop name for graph pack.
Posted by GitBox <gi...@apache.org>.
liangfu commented on issue #4703: [VTA] Support network which have no unique operator as start/stop name for graph pack.
URL: https://github.com/apache/incubator-tvm/pull/4703#issuecomment-577414054
+1, for default to `None` as ignore.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] liangfu commented on issue #4703: [VTA] Support
network which have no unique operator as start/stop name for graph pack.
Posted by GitBox <gi...@apache.org>.
liangfu commented on issue #4703: [VTA] Support network which have no unique operator as start/stop name for graph pack.
URL: https://github.com/apache/incubator-tvm/pull/4703#issuecomment-574013028
This is a favorable feature, since there could be more than one operator with the given name.
I wonder whether we can bring the unique name of the layer, instead of the name of the operator, to define the subgraph. I think using indices of the named operators are less intuitive, comparing to unique names of the layers.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] huajsj commented on issue #4703: [VTA] Support
network which have no unique operator as start/stop name for graph pack.
Posted by GitBox <gi...@apache.org>.
huajsj commented on issue #4703: [VTA] Support network which have no unique operator as start/stop name for graph pack.
URL: https://github.com/apache/incubator-tvm/pull/4703#issuecomment-574325187
Hi Liangfu, Thanks for the comments, the reason here to use operator name and indices is to first compatible with existing implementation like resnet18 example, secondly try to make the logic to
be more readable for that operator name is more friendly than indices only.
Regards
Hua
> This is a favorable feature, since there could be more than one operator with the given name.
>
> I wonder whether we can bring the unique name of the layer, instead of the name of the operator, to define the subgraph. I think using indices of the named operators are less intuitive, comparing to unique names of the layers.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] liangfu edited a comment on issue #4703: [VTA]
Support network which have no unique operator as start/stop name for graph
pack.
Posted by GitBox <gi...@apache.org>.
liangfu edited a comment on issue #4703: [VTA] Support network which have no unique operator as start/stop name for graph pack.
URL: https://github.com/apache/incubator-tvm/pull/4703#issuecomment-577414054
+1, for defaulting to `None` as ignore.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] huajsj commented on issue #4703: [VTA] Support
network which have no unique operator as start/stop name for graph pack.
Posted by GitBox <gi...@apache.org>.
huajsj commented on issue #4703: [VTA] Support network which have no unique operator as start/stop name for graph pack.
URL: https://github.com/apache/incubator-tvm/pull/4703#issuecomment-577340014
> Thank you @huajsj for addressing the comments. Is there a reason why we are defaulting to -1? Wouldn't that change the way we currently invoke graph_pack in our VTA examples?
Hi @tmoreau89 ,
thanks for the reply, the new solution use operator index instead of name replicate time to determine where is start/stop, hence 0 is a meaning value and can not as default value anymore, and i use -1 as the default value means skip index check.
this change no need any invoke/change for graph_pack in our VTA examples.
Regards
Hua
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] liangfu commented on a change in pull request
#4703: [VTA] Support network which have no unique operator as start/stop
name for graph pack.
Posted by GitBox <gi...@apache.org>.
liangfu commented on a change in pull request #4703: [VTA] Support network which have no unique operator as start/stop name for graph pack.
URL: https://github.com/apache/incubator-tvm/pull/4703#discussion_r367749989
##########
File path: vta/python/vta/top/graphpack.py
##########
@@ -283,15 +289,17 @@ def _recursion(anf, start_found, stop_found):
assert start_found
assert stop_found
return anf
- annotated = _recursion(anf, False, False)
+ annotated = _recursion(anf, False, False, operator_current_idx)
return run_opt_pass(annotated, transform.ToGraphNormalForm())
def graph_pack(expr,
bfactor,
cfactor,
weight_bits,
start_name="nn.max_pool2d",
- stop_name="nn.global_avg_pool2d"):
+ stop_name="nn.global_avg_pool2d",
+ start_name_idx=0,
Review comment:
should be equal to the defaults in `get_subgraph` function.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] liangfu commented on a change in pull request
#4703: [VTA] Support network which have no unique operator as start/stop
name for graph pack.
Posted by GitBox <gi...@apache.org>.
liangfu commented on a change in pull request #4703: [VTA] Support network which have no unique operator as start/stop name for graph pack.
URL: https://github.com/apache/incubator-tvm/pull/4703#discussion_r369882107
##########
File path: vta/python/vta/top/graphpack.py
##########
@@ -309,18 +329,30 @@ def graph_pack(expr,
The bit-width of the weights.
start_name: str, optional
- Start packing from certain known node.
+ Start packing from certain known node when start_name_idx is None.
stop_name: str, optional
- Stop packing from certain known node.
+ Stop packing from certain known node when stop_name_idx is None.
+
+ start_name_idx: str, optional
+ When start_name_idx not None, start packing only when node name equal start_name
+ and node idx equal start_name_idx.
+
+ stop_name_idx: str, optional
+ When stop_name_idx not None, stop packing only when node name equal stop_name
+ and node index equal stop_name_idx.
+
+ count_meta:boolean, optional
+ start_name_idx and stop_name_idx count meta or not.
Review comment:
Thanks for the clear explanation. I think it would be good to have the explanation in this line of 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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tmoreau89 commented on issue #4703: [VTA] Support
network which have no unique operator as start/stop name for graph pack.
Posted by GitBox <gi...@apache.org>.
tmoreau89 commented on issue #4703: [VTA] Support network which have no unique operator as start/stop name for graph pack.
URL: https://github.com/apache/incubator-tvm/pull/4703#issuecomment-577399577
I see, so in this case `-1` does not refer to "last" but instead refers to the default case, ignore?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tmoreau89 commented on issue #4703: [VTA] Support
network which have no unique operator as start/stop name for graph pack.
Posted by GitBox <gi...@apache.org>.
tmoreau89 commented on issue #4703: [VTA] Support network which have no unique operator as start/stop name for graph pack.
URL: https://github.com/apache/incubator-tvm/pull/4703#issuecomment-577332120
Thank you @huajsj for addressing the comments. Is there a reason why we are defaulting to -1? Wouldn't that change the way we currently invoke graph_pack in our VTA examples?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] huajsj edited a comment on issue #4703: [VTA]
Support network which have no unique operator as start/stop name for graph
pack.
Posted by GitBox <gi...@apache.org>.
huajsj edited a comment on issue #4703: [VTA] Support network which have no unique operator as start/stop name for graph pack.
URL: https://github.com/apache/incubator-tvm/pull/4703#issuecomment-577340014
> Thank you @huajsj for addressing the comments. Is there a reason why we are defaulting to -1? Wouldn't that change the way we currently invoke graph_pack in our VTA examples?
Hi @tmoreau89 ,
thanks for the reply, the new solution use operator index instead of name replicate time to determine where is start/stop, hence 0 is a meaning value and can not as default value anymore, and i use -1 as the default value means skip index check.
this change no need any invoke/change for graph_pack in our VTA examples.
Regards
Hua
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] liangfu commented on a change in pull request
#4703: [VTA] Support network which have no unique operator as start/stop
name for graph pack.
Posted by GitBox <gi...@apache.org>.
liangfu commented on a change in pull request #4703: [VTA] Support network which have no unique operator as start/stop name for graph pack.
URL: https://github.com/apache/incubator-tvm/pull/4703#discussion_r369881106
##########
File path: vta/python/vta/top/graphpack.py
##########
@@ -309,18 +329,30 @@ def graph_pack(expr,
The bit-width of the weights.
start_name: str, optional
- Start packing from certain known node.
+ Start packing from certain known node when start_name_idx is None.
stop_name: str, optional
- Stop packing from certain known node.
+ Stop packing from certain known node when stop_name_idx is None.
+
+ start_name_idx: str, optional
+ When start_name_idx not None, start packing only when node name equal start_name
+ and node idx equal start_name_idx.
Review comment:
I meant exactly "node index equals to start_name_idx", or am I wrong in the suggested English express?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] huajsj commented on a change in pull request #4703:
[VTA] Support network which have no unique operator as start/stop name for
graph pack.
Posted by GitBox <gi...@apache.org>.
huajsj commented on a change in pull request #4703: [VTA] Support network which have no unique operator as start/stop name for graph pack.
URL: https://github.com/apache/incubator-tvm/pull/4703#discussion_r367768015
##########
File path: vta/python/vta/top/graphpack.py
##########
@@ -246,32 +246,38 @@ def visit_call(self, call):
class BT(Exception):
pass
-def get_subgraph(expr, start_name, stop_name):
+def get_subgraph(expr, start_name, stop_name, start_name_idx=-1, stop_name_idx=-1):
Review comment:
I feel we may need to set both value is -1 to make the logic be consistent, would keep this no change.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] huajsj edited a comment on issue #4703: [VTA]
Support network which have no unique operator as start/stop name for graph
pack.
Posted by GitBox <gi...@apache.org>.
huajsj edited a comment on issue #4703: [VTA] Support network which have no unique operator as start/stop name for graph pack.
URL: https://github.com/apache/incubator-tvm/pull/4703#issuecomment-575405296
> I suggest we can follow a two-pass approach. For the first pass, get a list of named operator occurrence; then locate the subgraph by counting operator occurrence.
Hi @liangfu , I try this feature couple times, and feel the way to count operator occurrence times is not very convenience, and using the index directly is more straight forward and efficiency, the two-pass way may work but that add additional logic and make the solution too heavy, I would recommend to keep current one pass logic.
Regards
Hua
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] huajsj commented on a change in pull request #4703:
[VTA] Support network which have no unique operator as start/stop name for
graph pack.
Posted by GitBox <gi...@apache.org>.
huajsj commented on a change in pull request #4703: [VTA] Support network which have no unique operator as start/stop name for graph pack.
URL: https://github.com/apache/incubator-tvm/pull/4703#discussion_r367680469
##########
File path: vta/python/vta/top/graphpack.py
##########
@@ -246,32 +246,42 @@ def visit_call(self, call):
class BT(Exception):
pass
-def get_subgraph(expr, start_name, stop_name):
+def get_subgraph(expr, start_name, stop_name, start_name_indx=0, stop_name_indx=0):
Review comment:
fixed
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] huajsj commented on a change in pull request #4703:
[VTA] Support network which have no unique operator as start/stop name for
graph pack.
Posted by GitBox <gi...@apache.org>.
huajsj commented on a change in pull request #4703: [VTA] Support network which have no unique operator as start/stop name for graph pack.
URL: https://github.com/apache/incubator-tvm/pull/4703#discussion_r369853123
##########
File path: vta/python/vta/top/graphpack.py
##########
@@ -309,18 +329,30 @@ def graph_pack(expr,
The bit-width of the weights.
start_name: str, optional
- Start packing from certain known node.
+ Start packing from certain known node when start_name_idx is None.
stop_name: str, optional
- Stop packing from certain known node.
+ Stop packing from certain known node when stop_name_idx is None.
+
+ start_name_idx: str, optional
+ When start_name_idx not None, start packing only when node name equal start_name
+ and node idx equal start_name_idx.
+
+ stop_name_idx: str, optional
+ When stop_name_idx not None, stop packing only when node name equal stop_name
+ and node index equal stop_name_idx.
+
+ count_meta:boolean, optional
+ start_name_idx and stop_name_idx count meta or not.
Review comment:
operator can be relay.expr.Constant which is meta data, default the operator increase logic would not count the meta when count_meta is False , the index number used in graph_pack following the index from 'print(mod.astext(show_meta_data=False))' .
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] liangfu commented on a change in pull request
#4703: [VTA] Support network which have no unique operator as start/stop
name for graph pack.
Posted by GitBox <gi...@apache.org>.
liangfu commented on a change in pull request #4703: [VTA] Support network which have no unique operator as start/stop name for graph pack.
URL: https://github.com/apache/incubator-tvm/pull/4703#discussion_r369848344
##########
File path: vta/python/vta/top/graphpack.py
##########
@@ -309,18 +329,30 @@ def graph_pack(expr,
The bit-width of the weights.
start_name: str, optional
- Start packing from certain known node.
+ Start packing from certain known node when start_name_idx is None.
stop_name: str, optional
- Stop packing from certain known node.
+ Stop packing from certain known node when stop_name_idx is None.
+
+ start_name_idx: str, optional
+ When start_name_idx not None, start packing only when node name equal start_name
+ and node idx equal start_name_idx.
+
+ stop_name_idx: str, optional
+ When stop_name_idx not None, stop packing only when node name equal stop_name
+ and node index equal stop_name_idx.
+
+ count_meta:boolean, optional
+ start_name_idx and stop_name_idx count meta or not.
Review comment:
Can you explain what is the meta here?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tmoreau89 commented on a change in pull request
#4703: [VTA] Support network which have no unique operator as start/stop
name for graph pack.
Posted by GitBox <gi...@apache.org>.
tmoreau89 commented on a change in pull request #4703: [VTA] Support network which have no unique operator as start/stop name for graph pack.
URL: https://github.com/apache/incubator-tvm/pull/4703#discussion_r367177369
##########
File path: vta/python/vta/top/graphpack.py
##########
@@ -246,32 +246,42 @@ def visit_call(self, call):
class BT(Exception):
pass
-def get_subgraph(expr, start_name, stop_name):
+def get_subgraph(expr, start_name, stop_name, start_name_indx=0, stop_name_indx=0):
Review comment:
Agree, please use `idx`
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tmoreau89 commented on issue #4703: [VTA] Support
network which have no unique operator as start/stop name for graph pack.
Posted by GitBox <gi...@apache.org>.
tmoreau89 commented on issue #4703: [VTA] Support network which have no unique operator as start/stop name for graph pack.
URL: https://github.com/apache/incubator-tvm/pull/4703#issuecomment-577407734
Ok, can we then switch the default to `None` in order to ensure that `-1` is not mistaken for "last" in numpy fashion?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] liangfu commented on issue #4703: [VTA] Support
network which have no unique operator as start/stop name for graph pack.
Posted by GitBox <gi...@apache.org>.
liangfu commented on issue #4703: [VTA] Support network which have no unique operator as start/stop name for graph pack.
URL: https://github.com/apache/incubator-tvm/pull/4703#issuecomment-575401230
I suggest we can follow a two-pass approach. For the first pass, get a list of named operator occurrence; then locate the subgraph by counting operator occurrence.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] huajsj commented on a change in pull request #4703:
[VTA] Support network which have no unique operator as start/stop name for
graph pack.
Posted by GitBox <gi...@apache.org>.
huajsj commented on a change in pull request #4703: [VTA] Support network which have no unique operator as start/stop name for graph pack.
URL: https://github.com/apache/incubator-tvm/pull/4703#discussion_r370328795
##########
File path: vta/python/vta/top/graphpack.py
##########
@@ -309,18 +329,30 @@ def graph_pack(expr,
The bit-width of the weights.
start_name: str, optional
- Start packing from certain known node.
+ Start packing from certain known node when start_name_idx is None.
stop_name: str, optional
- Stop packing from certain known node.
+ Stop packing from certain known node when stop_name_idx is None.
+
+ start_name_idx: int, optional
+ When start_name_idx not None, start packing only when node name equal start_name
+ and node idx equals start_name_idx.
+
+ stop_name_idx: int, optional
+ When stop_name_idx not None, stop packing only when node name equal stop_name
+ and node index equals stop_name_idx.
+
+ count_meta:boolean, optional
+ start_name_idx and stop_name_idx count meta or not.
Review comment:
sure, fixed.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] liangfu commented on a change in pull request
#4703: [VTA] Support network which have no unique operator as start/stop
name for graph pack.
Posted by GitBox <gi...@apache.org>.
liangfu commented on a change in pull request #4703: [VTA] Support network which have no unique operator as start/stop name for graph pack.
URL: https://github.com/apache/incubator-tvm/pull/4703#discussion_r366724607
##########
File path: vta/python/vta/top/graphpack.py
##########
@@ -246,32 +246,42 @@ def visit_call(self, call):
class BT(Exception):
pass
-def get_subgraph(expr, start_name, stop_name):
+def get_subgraph(expr, start_name, stop_name, start_name_indx=0, stop_name_indx=0):
Review comment:
is this a typo? the abbreviation for 'index' should be either 'ind', 'idx', or 'index'.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tmoreau89 merged pull request #4703: [VTA] Support
network which have no unique operator as start/stop name for graph pack.
Posted by GitBox <gi...@apache.org>.
tmoreau89 merged pull request #4703: [VTA] Support network which have no unique operator as start/stop name for graph pack.
URL: https://github.com/apache/incubator-tvm/pull/4703
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] huajsj commented on issue #4703: [VTA] Support
network which have no unique operator as start/stop name for graph pack.
Posted by GitBox <gi...@apache.org>.
huajsj commented on issue #4703: [VTA] Support network which have no unique operator as start/stop name for graph pack.
URL: https://github.com/apache/incubator-tvm/pull/4703#issuecomment-577327346
Hi @tmoreau89 @liangfu , all review comments get addressed, please let me know if have any more comments. Thanks.
Regards
Hua
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] liangfu commented on a change in pull request
#4703: [VTA] Support network which have no unique operator as start/stop
name for graph pack.
Posted by GitBox <gi...@apache.org>.
liangfu commented on a change in pull request #4703: [VTA] Support network which have no unique operator as start/stop name for graph pack.
URL: https://github.com/apache/incubator-tvm/pull/4703#discussion_r369846087
##########
File path: vta/python/vta/top/graphpack.py
##########
@@ -309,18 +329,30 @@ def graph_pack(expr,
The bit-width of the weights.
start_name: str, optional
- Start packing from certain known node.
+ Start packing from certain known node when start_name_idx is None.
stop_name: str, optional
- Stop packing from certain known node.
+ Stop packing from certain known node when stop_name_idx is None.
+
+ start_name_idx: str, optional
+ When start_name_idx not None, start packing only when node name equal start_name
+ and node idx equal start_name_idx.
Review comment:
typo: node index equals to start_name_idx
some other similar expressions should be updated as well.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] liangfu commented on a change in pull request
#4703: [VTA] Support network which have no unique operator as start/stop
name for graph pack.
Posted by GitBox <gi...@apache.org>.
liangfu commented on a change in pull request #4703: [VTA] Support network which have no unique operator as start/stop name for graph pack.
URL: https://github.com/apache/incubator-tvm/pull/4703#discussion_r369846087
##########
File path: vta/python/vta/top/graphpack.py
##########
@@ -309,18 +329,30 @@ def graph_pack(expr,
The bit-width of the weights.
start_name: str, optional
- Start packing from certain known node.
+ Start packing from certain known node when start_name_idx is None.
stop_name: str, optional
- Stop packing from certain known node.
+ Stop packing from certain known node when stop_name_idx is None.
+
+ start_name_idx: str, optional
+ When start_name_idx not None, start packing only when node name equal start_name
+ and node idx equal start_name_idx.
Review comment:
node index equals to start_name_idx
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] huajsj commented on issue #4703: [VTA] Support
network which have no unique operator as start/stop name for graph pack.
Posted by GitBox <gi...@apache.org>.
huajsj commented on issue #4703: [VTA] Support network which have no unique operator as start/stop name for graph pack.
URL: https://github.com/apache/incubator-tvm/pull/4703#issuecomment-575405296
> I suggest we can follow a two-pass approach. For the first pass, get a list of named operator occurrence; then locate the subgraph by counting operator occurrence.
Hi @liangfu , I try this feature couple times, and feel the way to count operator occurrence times is not very convenience, and using the index directly is more straight forward and efficiency, the two-pass way may work but that add additional logic and make the solution too heavy, I would recommend to keep current one pass logic.
Regards
Hua
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tmoreau89 commented on a change in pull request
#4703: [VTA] Support network which have no unique operator as start/stop
name for graph pack.
Posted by GitBox <gi...@apache.org>.
tmoreau89 commented on a change in pull request #4703: [VTA] Support network which have no unique operator as start/stop name for graph pack.
URL: https://github.com/apache/incubator-tvm/pull/4703#discussion_r370317355
##########
File path: vta/python/vta/top/graphpack.py
##########
@@ -309,18 +329,30 @@ def graph_pack(expr,
The bit-width of the weights.
start_name: str, optional
- Start packing from certain known node.
+ Start packing from certain known node when start_name_idx is None.
stop_name: str, optional
- Stop packing from certain known node.
+ Stop packing from certain known node when stop_name_idx is None.
+
+ start_name_idx: int, optional
+ When start_name_idx not None, start packing only when node name equal start_name
+ and node idx equals start_name_idx.
+
+ stop_name_idx: int, optional
+ When stop_name_idx not None, stop packing only when node name equal stop_name
+ and node index equals stop_name_idx.
+
+ count_meta:boolean, optional
+ start_name_idx and stop_name_idx count meta or not.
Review comment:
can this description be improved according to your response to @liangfu ?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] huajsj commented on issue #4703: [VTA] Support
network which have no unique operator as start/stop name for graph pack.
Posted by GitBox <gi...@apache.org>.
huajsj commented on issue #4703: [VTA] Support network which have no unique operator as start/stop name for graph pack.
URL: https://github.com/apache/incubator-tvm/pull/4703#issuecomment-573966672
@tmoreau89 could you help for a review? 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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] liangfu commented on a change in pull request
#4703: [VTA] Support network which have no unique operator as start/stop
name for graph pack.
Posted by GitBox <gi...@apache.org>.
liangfu commented on a change in pull request #4703: [VTA] Support network which have no unique operator as start/stop name for graph pack.
URL: https://github.com/apache/incubator-tvm/pull/4703#discussion_r367772171
##########
File path: vta/python/vta/top/graphpack.py
##########
@@ -246,32 +246,38 @@ def visit_call(self, call):
class BT(Exception):
pass
-def get_subgraph(expr, start_name, stop_name):
+def get_subgraph(expr, start_name, stop_name, start_name_idx=-1, stop_name_idx=-1):
Review comment:
I think there might be a misunderstanding.
Let's take a specific example. For a network with layers defined as
```
%0 = nn.conv2d
%1 = nn.batch_norm
%2 = nn.relu
%3 = nn.max_pool2d
%4 = nn.conv2d
%5 = nn.batch_norm
%6 = nn.relu
%7 = nn.max_pool2d
%8 = nn.conv2d
%9 = nn.global_avg_pool2d
%10 = nn.dense
%11 = nn.softmax
```
, we use your default configuration which means start_name="nn.max_pool2d", stop_name="nn.global_avg_pool2d", start_name_idx=-1, stop_name_idx=-1. What you would get would be the subgraph between %7 and %9. So, I'm suggesting setting `start_name_idx`=0, so that we could have the subgraph extracted between %3 and %9, which I think would be more reasonable.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] liangfu commented on a change in pull request
#4703: [VTA] Support network which have no unique operator as start/stop
name for graph pack.
Posted by GitBox <gi...@apache.org>.
liangfu commented on a change in pull request #4703: [VTA] Support network which have no unique operator as start/stop name for graph pack.
URL: https://github.com/apache/incubator-tvm/pull/4703#discussion_r366724818
##########
File path: vta/python/vta/top/graphpack.py
##########
@@ -246,32 +246,42 @@ def visit_call(self, call):
class BT(Exception):
pass
-def get_subgraph(expr, start_name, stop_name):
+def get_subgraph(expr, start_name, stop_name, start_name_indx=0, stop_name_indx=0):
""" We assume stop_name only appears once for simplicity.
This constraint will be lifted in the future.
bitpack_start and bitpack_end are both inclusive.
"""
bitpack_start = op.op.get('annotation.bitpack_start')
bitpack_end = op.op.get('annotation.bitpack_end')
anf = run_opt_pass(expr, transform.ToANormalForm())
- def _recursion(anf, start_found, stop_found):
+ start_name_num = 0
+ stop_name_num = 0
+ def _recursion(anf, start_found, stop_found, start_name_num, stop_name_num):
Review comment:
CHECK: when start_name==stopname, start index should be smaller than stop index.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] liangfu commented on a change in pull request
#4703: [VTA] Support network which have no unique operator as start/stop
name for graph pack.
Posted by GitBox <gi...@apache.org>.
liangfu commented on a change in pull request #4703: [VTA] Support network which have no unique operator as start/stop name for graph pack.
URL: https://github.com/apache/incubator-tvm/pull/4703#discussion_r367749718
##########
File path: vta/python/vta/top/graphpack.py
##########
@@ -246,32 +246,38 @@ def visit_call(self, call):
class BT(Exception):
pass
-def get_subgraph(expr, start_name, stop_name):
+def get_subgraph(expr, start_name, stop_name, start_name_idx=-1, stop_name_idx=-1):
Review comment:
I suggest setting default `start_name_idx` being 0 (first occurrence of the operator), while keeping `stop_name_idx` being -1 (last occurrence).
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] huajsj commented on issue #4703: [VTA] Support
network which have no unique operator as start/stop name for graph pack.
Posted by GitBox <gi...@apache.org>.
huajsj commented on issue #4703: [VTA] Support network which have no unique operator as start/stop name for graph pack.
URL: https://github.com/apache/incubator-tvm/pull/4703#issuecomment-577418290
Hi @tmoreau89 , @liangfu , change already done. thanks.
Regards
Hua
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] huajsj commented on a change in pull request #4703:
[VTA] Support network which have no unique operator as start/stop name for
graph pack.
Posted by GitBox <gi...@apache.org>.
huajsj commented on a change in pull request #4703: [VTA] Support network which have no unique operator as start/stop name for graph pack.
URL: https://github.com/apache/incubator-tvm/pull/4703#discussion_r369853302
##########
File path: vta/python/vta/top/graphpack.py
##########
@@ -309,18 +329,30 @@ def graph_pack(expr,
The bit-width of the weights.
start_name: str, optional
- Start packing from certain known node.
+ Start packing from certain known node when start_name_idx is None.
stop_name: str, optional
- Stop packing from certain known node.
+ Stop packing from certain known node when stop_name_idx is None.
+
+ start_name_idx: str, optional
+ When start_name_idx not None, start packing only when node name equal start_name
+ and node idx equal start_name_idx.
Review comment:
fixed.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] liangfu commented on a change in pull request
#4703: [VTA] Support network which have no unique operator as start/stop
name for graph pack.
Posted by GitBox <gi...@apache.org>.
liangfu commented on a change in pull request #4703: [VTA] Support network which have no unique operator as start/stop name for graph pack.
URL: https://github.com/apache/incubator-tvm/pull/4703#discussion_r369846920
##########
File path: vta/python/vta/top/graphpack.py
##########
@@ -309,18 +329,30 @@ def graph_pack(expr,
The bit-width of the weights.
start_name: str, optional
- Start packing from certain known node.
+ Start packing from certain known node when start_name_idx is None.
stop_name: str, optional
- Stop packing from certain known node.
+ Stop packing from certain known node when stop_name_idx is None.
+
+ start_name_idx: str, optional
Review comment:
The index should be int?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] huajsj commented on issue #4703: [VTA] Support
network which have no unique operator as start/stop name for graph pack.
Posted by GitBox <gi...@apache.org>.
huajsj commented on issue #4703: [VTA] Support network which have no unique operator as start/stop name for graph pack.
URL: https://github.com/apache/incubator-tvm/pull/4703#issuecomment-577406008
> I see, so in this case `-1` does not refer to "last" but instead refers to the default case, ignore?
Hi @tmoreau89 , yes, -1 is ignore, for the requirement to refer last , here we can directly use the index number.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tmoreau89 commented on issue #4703: [VTA] Support
network which have no unique operator as start/stop name for graph pack.
Posted by GitBox <gi...@apache.org>.
tmoreau89 commented on issue #4703: [VTA] Support network which have no unique operator as start/stop name for graph pack.
URL: https://github.com/apache/incubator-tvm/pull/4703#issuecomment-577899433
Thank you @huajsj @liangfu , the PR has been merged.
----------------------------------------------------------------
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
With regards,
Apache Git Services