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/15 06:28:20 UTC
[GitHub] [incubator-tvm] moderato opened a new pull request #5811: [AutoTVM][Fix] Fixed 'not in list' error for define_reorder with 'candidate' policy.
moderato opened a new pull request #5811:
URL: https://github.com/apache/incubator-tvm/pull/5811
Hello! This is my first time sending a pull-request. I found when the `candidate` policy is chosen for `define_reorder`, AutoTVM throws an error of `xxx not in list`. It's because in
https://github.com/apache/incubator-tvm/blob/d0f988988a6f4875d176da1d79628feebf2ee023/python/tvm/autotvm/task/space.py#L313
and other similar places, `x` has the type of `iter_var` while elements in `axes` have the type of `VirtualAxis`. This pull request fixes the bug.
A review would be much appreciated! @tqchen @comaniac @merrymercy
----------------------------------------------------------------
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] merrymercy edited a comment on pull request #5811: [AutoTVM] Support using IterVar directly in define_reorder with 'candidate' policy.
Posted by GitBox <gi...@apache.org>.
merrymercy edited a comment on pull request #5811:
URL: https://github.com/apache/incubator-tvm/pull/5811#issuecomment-645703625
Users should do the conversion from `IterVar` to `VirtualAxis` by themself in the templates.
Then the equality check follows the default semantics of python, which is check-by-reference.
Then the `candidates` argument works correctly.
Checking equality by name is not desired. I close this PR for now. Feel free to reopen if you have better equality check strategies.
----------------------------------------------------------------
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] merrymercy edited a comment on pull request #5811: [AutoTVM][Fix] Fixed 'not in list' error for define_reorder with 'candidate' policy.
Posted by GitBox <gi...@apache.org>.
merrymercy edited a comment on pull request #5811:
URL: https://github.com/apache/incubator-tvm/pull/5811#issuecomment-645690245
Because the `name` is only used for printing and debugging, it does not matter.
I am saying the ideal way is that you create VirtualAxis in your templates explicitly and do not reply on `name` to do the equality check.
So we can even close this PR. The existing code does not have bugs.
----------------------------------------------------------------
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] merrymercy edited a comment on pull request #5811: [AutoTVM] Support using IterVar directly in define_reorder with 'candidate' policy.
Posted by GitBox <gi...@apache.org>.
merrymercy edited a comment on pull request #5811:
URL: https://github.com/apache/incubator-tvm/pull/5811#issuecomment-645702581
Users should do the conversion from `IterVar` to `VirtualAxis` by themself in the templates.
Then the equality check follows the default semantics of python, which is check by reference.
Then the `candidates` argument works correctly.
If you are happy with this design, you can close this PR. Otherwise, the priority of which `name` to choose does no matter to me. I am okay with your current 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] merrymercy removed a comment on pull request #5811: [AutoTVM] Support using IterVar directly in define_reorder with 'candidate' policy.
Posted by GitBox <gi...@apache.org>.
merrymercy removed a comment on pull request #5811:
URL: https://github.com/apache/incubator-tvm/pull/5811#issuecomment-645702581
Users should do the conversion from `IterVar` to `VirtualAxis` by themself in the templates.
Then the equality check follows the default semantics of python, which is check by reference.
Then the `candidates` argument works correctly.
If you are happy with this design, you can close this PR. Otherwise, the priority of which `name` to choose does no matter to me. I am okay with your current 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] comaniac commented on a change in pull request #5811: [AutoTVM][Fix] Fixed 'not in list' error for define_reorder with 'candidate' policy.
Posted by GitBox <gi...@apache.org>.
comaniac commented on a change in pull request #5811:
URL: https://github.com/apache/incubator-tvm/pull/5811#discussion_r440991097
##########
File path: python/tvm/autotvm/task/space.py
##########
@@ -119,21 +127,23 @@ def __init__(self, var, name=None):
super(VirtualAxis, self).__init__()
self.num_output = 1
- if name is None:
- name = 'axis_%d' % VirtualAxis.name_ct
- VirtualAxis.name_ct += 1
-
self.name = name
if isinstance(var, (int, _long)):
self.length = var
+ if name is None:
Review comment:
Use `self.name` to make it consistent.
----------------------------------------------------------------
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] comaniac commented on pull request #5811: [AutoTVM][Fix] Fixed 'not in list' error for define_reorder with 'candidate' policy.
Posted by GitBox <gi...@apache.org>.
comaniac commented on pull request #5811:
URL: https://github.com/apache/incubator-tvm/pull/5811#issuecomment-644317657
> @comaniac A quick replication of the bug is to replace
> https://github.com/apache/incubator-tvm/blob/520ca0a8b39aeb4765369f169477265230ea7c6c/tutorials/autotvm/tune_simple_template.py#L209
>
>
> with
> ```
> cfg.define_reorder('reorder', [yo, xo, k, yi, xi], policy='candidate', candidate=[[yo, xo, k, yi, xi], [xo, yo, k, yi, xi]])
> cfg['reorder'].apply(s, C, [yo, xo, k, yi, xi])
> ```
I see what you meant. It seems like all use cases in current TOPI reorder only the splitted axises, which are all virtual axises already. Accordingly, I'd suggest checking and creating `VirtualAxis` for `axes` in the beginning of `ReorderSpace.__init__`.
----------------------------------------------------------------
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] merrymercy closed pull request #5811: [AutoTVM] Support using IterVar directly in define_reorder with 'candidate' policy.
Posted by GitBox <gi...@apache.org>.
merrymercy closed pull request #5811:
URL: https://github.com/apache/incubator-tvm/pull/5811
----------------------------------------------------------------
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] merrymercy edited a comment on pull request #5811: [AutoTVM][Fix] Fixed 'not in list' error for define_reorder with 'candidate' policy.
Posted by GitBox <gi...@apache.org>.
merrymercy edited a comment on pull request #5811:
URL: https://github.com/apache/incubator-tvm/pull/5811#issuecomment-645690245
Because the `name` is only used for printing and debugging, it does not matter.
I am saying the ideal way is that you create VirtualAxis in your templates explicitly and do not reply on `name` to do the equality check.
So we can even close this PR.
----------------------------------------------------------------
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] merrymercy edited a comment on pull request #5811: [AutoTVM][Fix] Fixed 'not in list' error for define_reorder with 'candidate' policy.
Posted by GitBox <gi...@apache.org>.
merrymercy edited a comment on pull request #5811:
URL: https://github.com/apache/incubator-tvm/pull/5811#issuecomment-645602540
The `name` is just for printing and debugging. Ideally, you should not use it for the equality check.
You should only create one VirtualAxis for one IterVar.
----------------------------------------------------------------
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] merrymercy commented on pull request #5811: [AutoTVM] Support using IterVar directly in define_reorder with 'candidate' policy.
Posted by GitBox <gi...@apache.org>.
merrymercy commented on pull request #5811:
URL: https://github.com/apache/incubator-tvm/pull/5811#issuecomment-645702581
Users should do the conversion from `IterVar` to `VirtualAxis` by themself in the templates.
Then the equality check follows the default semantics of python, which is check by reference.
Then the `candidates` argument works correctly.
If you are happy with this design, you can close this PR. Otherwise, the priority of 'name' does no matter to me, you can choose either.
----------------------------------------------------------------
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] merrymercy edited a comment on pull request #5811: [AutoTVM] Support using IterVar directly in define_reorder with 'candidate' policy.
Posted by GitBox <gi...@apache.org>.
merrymercy edited a comment on pull request #5811:
URL: https://github.com/apache/incubator-tvm/pull/5811#issuecomment-645690245
----------------------------------------------------------------
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] moderato commented on pull request #5811: [AutoTVM][Fix] Fixed 'not in list' error for define_reorder with 'candidate' policy.
Posted by GitBox <gi...@apache.org>.
moderato commented on pull request #5811:
URL: https://github.com/apache/incubator-tvm/pull/5811#issuecomment-644583322
> > @comaniac A quick replication of the bug is to replace
> > https://github.com/apache/incubator-tvm/blob/520ca0a8b39aeb4765369f169477265230ea7c6c/tutorials/autotvm/tune_simple_template.py#L209
> >
> > with
> > ```
> > cfg.define_reorder('reorder', [yo, xo, k, yi, xi], policy='candidate', candidate=[[yo, xo, k, yi, xi], [xo, yo, k, yi, xi]])
> > cfg['reorder'].apply(s, C, [yo, xo, k, yi, xi])
> > ```
>
> I see what you meant. It seems like all use cases in current TOPI reorder only the splitted axises, which are all virtual axises already. Accordingly, I'd suggest checking and creating `VirtualAxis` for `axes` in the beginning of `ReorderSpace.__init__`.
Yeah, actually `axes` are all good. I add a few lines of type checking code for some of the policies.
----------------------------------------------------------------
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] merrymercy commented on pull request #5811: [AutoTVM][Fix] Fixed 'not in list' error for define_reorder with 'candidate' policy.
Posted by GitBox <gi...@apache.org>.
merrymercy commented on pull request #5811:
URL: https://github.com/apache/incubator-tvm/pull/5811#issuecomment-645563545
In the original design and underlying implementation, All APIs should only accept VirtualAxis.
Later, we make some APIs support IterVar as sugar.
We can merge this. Thanks @moderato and @comaniac
----------------------------------------------------------------
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] merrymercy edited a comment on pull request #5811: [AutoTVM] Support using IterVar directly in define_reorder with 'candidate' policy.
Posted by GitBox <gi...@apache.org>.
merrymercy edited a comment on pull request #5811:
URL: https://github.com/apache/incubator-tvm/pull/5811#issuecomment-645690245
Because the `name` is only used for printing and debugging, it does not matter.
The correct way is that you create VirtualAxis in your templates explicitly and do not rely on `name` to do the equality check.
If you agree with this, we can close the PR. The existing code does not have bugs.
----------------------------------------------------------------
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] merrymercy edited a comment on pull request #5811: [AutoTVM][Fix] Fixed 'not in list' error for define_reorder with 'candidate' policy.
Posted by GitBox <gi...@apache.org>.
merrymercy edited a comment on pull request #5811:
URL: https://github.com/apache/incubator-tvm/pull/5811#issuecomment-645690245
Because the 'name' is only used for printing and debugging. It does not matter.
I am saying the ideal way is that you create VirtualAxis in your templates explicitly and do not reply on `name` to do the equality check.
So we can even close this PR.
----------------------------------------------------------------
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] moderato commented on a change in pull request #5811: [AutoTVM][Fix] Fixed 'not in list' error for define_reorder with 'candidate' policy.
Posted by GitBox <gi...@apache.org>.
moderato commented on a change in pull request #5811:
URL: https://github.com/apache/incubator-tvm/pull/5811#discussion_r440636860
##########
File path: python/tvm/autotvm/task/space.py
##########
@@ -119,21 +125,26 @@ def __init__(self, var, name=None):
super(VirtualAxis, self).__init__()
self.num_output = 1
- if name is None:
- name = 'axis_%d' % VirtualAxis.name_ct
- VirtualAxis.name_ct += 1
+ self.name = None
+ if name is not None:
+ self.name = name
- self.name = name
if isinstance(var, (int, _long)):
self.length = var
+ if name is None:
+ self.name = 'axis_%d' % VirtualAxis.name_ct
+ VirtualAxis.name_ct += 1
elif isinstance(var, schedule.IterVar):
- self.name = var.var.name
+ if self.name is None:
+ self.name = var.var.name
if var.dom is None:
self.length = -1
else:
self.length = get_const_int(var.dom.extent)
elif isinstance(var, VirtualAxis):
Review comment:
Done.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] merrymercy edited a comment on pull request #5811: [AutoTVM][Fix] Fixed 'not in list' error for define_reorder with 'candidate' policy.
Posted by GitBox <gi...@apache.org>.
merrymercy edited a comment on pull request #5811:
URL: https://github.com/apache/incubator-tvm/pull/5811#issuecomment-645602540
The `name` is just for printing and debugging. Ideally, you should not use it for the equality check.
----------------------------------------------------------------
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] moderato commented on pull request #5811: [AutoTVM][Fix] Fixed 'not in list' error for define_reorder with 'candidate' policy.
Posted by GitBox <gi...@apache.org>.
moderato commented on pull request #5811:
URL: https://github.com/apache/incubator-tvm/pull/5811#issuecomment-645567470
> In the original design and underlying implementation, all APIs should only accept VirtualAxis.
> Later, we make some APIs support IterVar as sugar.
>
> @moderato Please fix the CI.
@merrymercy Thank you for the review! Can you clarify the semantics of naming a VirtualAxis (see above)? Our confusion is if the `name` argument of the constructor should dominate when it's not None and`var` has a type of `IterVar` or `VirtualAxis` that has a name.
----------------------------------------------------------------
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] merrymercy edited a comment on pull request #5811: [AutoTVM] Support using IterVar directly in define_reorder with 'candidate' policy.
Posted by GitBox <gi...@apache.org>.
merrymercy edited a comment on pull request #5811:
URL: https://github.com/apache/incubator-tvm/pull/5811#issuecomment-645703625
Users should do the conversion from `IterVar` to `VirtualAxis` by themself in the templates.
Then the equality check follows the default semantics of python, which is check-by-reference.
Then the `candidates` argument works correctly.
Checking equality by name is not desired. I close this PR for now. Feel free to reopen if you have better equality check strategies.
The `name_ct` is just for pretty printing.
----------------------------------------------------------------
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] moderato commented on a change in pull request #5811: [AutoTVM][Fix] Fixed 'not in list' error for define_reorder with 'candidate' policy.
Posted by GitBox <gi...@apache.org>.
moderato commented on a change in pull request #5811:
URL: https://github.com/apache/incubator-tvm/pull/5811#discussion_r440637088
##########
File path: python/tvm/autotvm/task/space.py
##########
@@ -119,21 +125,26 @@ def __init__(self, var, name=None):
super(VirtualAxis, self).__init__()
self.num_output = 1
- if name is None:
- name = 'axis_%d' % VirtualAxis.name_ct
- VirtualAxis.name_ct += 1
+ self.name = None
+ if name is not None:
+ self.name = name
Review comment:
Done.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] merrymercy edited a comment on pull request #5811: [AutoTVM][Fix] Fixed 'not in list' error for define_reorder with 'candidate' policy.
Posted by GitBox <gi...@apache.org>.
merrymercy edited a comment on pull request #5811:
URL: https://github.com/apache/incubator-tvm/pull/5811#issuecomment-645563545
In the original design and underlying implementation, all APIs should only accept VirtualAxis.
Later, we make some APIs support IterVar as sugar.
Please fix the CI.
----------------------------------------------------------------
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] merrymercy commented on pull request #5811: [AutoTVM] Support using IterVar directly in define_reorder with 'candidate' policy.
Posted by GitBox <gi...@apache.org>.
merrymercy commented on pull request #5811:
URL: https://github.com/apache/incubator-tvm/pull/5811#issuecomment-645703625
Users should do the conversion from `IterVar` to `VirtualAxis` by themself in the templates.
Then the equality check follows the default semantics of python, which is check by reference.
Then the `candidates` argument works correctly.
Checking by name is not desired.
----------------------------------------------------------------
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] moderato commented on pull request #5811: [AutoTVM][Fix] Fixed 'not in list' error for define_reorder with 'candidate' policy.
Posted by GitBox <gi...@apache.org>.
moderato commented on pull request #5811:
URL: https://github.com/apache/incubator-tvm/pull/5811#issuecomment-644306923
@comaniac A quick replication of the bug is to replace
https://github.com/apache/incubator-tvm/blob/520ca0a8b39aeb4765369f169477265230ea7c6c/tutorials/autotvm/tune_simple_template.py#L209
with
```
cfg.define_reorder('reorder', [yo, xo, k, yi, xi], policy='candidate', candidate=[[yo, xo, k, yi, xi], [xo, yo, k, yi, xi]])
cfg['reorder'].apply(s, C, [yo, xo, k, yi, xi])
```
----------------------------------------------------------------
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] moderato commented on pull request #5811: [AutoTVM] Support using IterVar directly in define_reorder with 'candidate' policy.
Posted by GitBox <gi...@apache.org>.
moderato commented on pull request #5811:
URL: https://github.com/apache/incubator-tvm/pull/5811#issuecomment-645699015
> Because the `name` is only used for printing and debugging, it does not matter.
> The correct way is that you create VirtualAxis in your templates explicitly and do not rely on `name` to do the equality check.
>
> If you agree with this, we can close the PR. The existing code does not have bugs.
Well, the `name_ct` variable does affect the equality check at places like
https://github.com/apache/incubator-tvm/blob/052ea4d14d4a528ae7a2c1f39e74436284864d39/python/tvm/autotvm/task/space.py#L313
when `var` is an `IterVar` and needed to be converted to `VirtualAxis`. That's why I was asking about `name`. Or, would you suggest to change the equality check codes?
However, if the plan to support `IterVar` is to be done in the future instead of now, I agree we can close this PR 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] moderato commented on a change in pull request #5811: [AutoTVM][Fix] Fixed 'not in list' error for define_reorder with 'candidate' policy.
Posted by GitBox <gi...@apache.org>.
moderato commented on a change in pull request #5811:
URL: https://github.com/apache/incubator-tvm/pull/5811#discussion_r440362082
##########
File path: python/tvm/autotvm/task/space.py
##########
@@ -119,21 +125,26 @@ def __init__(self, var, name=None):
super(VirtualAxis, self).__init__()
self.num_output = 1
- if name is None:
- name = 'axis_%d' % VirtualAxis.name_ct
- VirtualAxis.name_ct += 1
+ self.name = None
+ if name is not None:
+ self.name = name
- self.name = name
if isinstance(var, (int, _long)):
self.length = var
+ if name is None:
+ self.name = 'axis_%d' % VirtualAxis.name_ct
+ VirtualAxis.name_ct += 1
elif isinstance(var, schedule.IterVar):
- self.name = var.var.name
+ if self.name is None:
+ self.name = var.var.name
if var.dom is None:
self.length = -1
else:
self.length = get_const_int(var.dom.extent)
elif isinstance(var, VirtualAxis):
self.length = var.length
+ if self.name is None:
+ self.name = var.name
Review comment:
Actually I'm a bit confused by the original semantic. It meant to overwrite the name with var's name if var is an IterVar, but not to overwrite if var is a VirtualAxis which already has a name. Plus, when name=None and var is an IterVar, name_ct increments by 1 but it's not used in naming. Is this what it is supposed to be?
----------------------------------------------------------------
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] merrymercy edited a comment on pull request #5811: [AutoTVM] Support using IterVar directly in define_reorder with 'candidate' policy.
Posted by GitBox <gi...@apache.org>.
merrymercy edited a comment on pull request #5811:
URL: https://github.com/apache/incubator-tvm/pull/5811#issuecomment-645690245
Because the `name` is only used for printing and debugging, it does not matter.
The ideal way is that you create VirtualAxis in your templates explicitly and do not reply on `name` to do the equality check.
So we can even close this PR. The existing code does not have bugs.
----------------------------------------------------------------
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] merrymercy commented on pull request #5811: [AutoTVM][Fix] Fixed 'not in list' error for define_reorder with 'candidate' policy.
Posted by GitBox <gi...@apache.org>.
merrymercy commented on pull request #5811:
URL: https://github.com/apache/incubator-tvm/pull/5811#issuecomment-645602540
The `name` is just for printing and debugging. Ideally you should not use it for equality check.
----------------------------------------------------------------
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] comaniac commented on a change in pull request #5811: [AutoTVM][Fix] Fixed 'not in list' error for define_reorder with 'candidate' policy.
Posted by GitBox <gi...@apache.org>.
comaniac commented on a change in pull request #5811:
URL: https://github.com/apache/incubator-tvm/pull/5811#discussion_r440383211
##########
File path: python/tvm/autotvm/task/space.py
##########
@@ -119,21 +125,26 @@ def __init__(self, var, name=None):
super(VirtualAxis, self).__init__()
self.num_output = 1
- if name is None:
- name = 'axis_%d' % VirtualAxis.name_ct
- VirtualAxis.name_ct += 1
+ self.name = None
+ if name is not None:
+ self.name = name
- self.name = name
if isinstance(var, (int, _long)):
self.length = var
+ if name is None:
+ self.name = 'axis_%d' % VirtualAxis.name_ct
+ VirtualAxis.name_ct += 1
elif isinstance(var, schedule.IterVar):
- self.name = var.var.name
+ if self.name is None:
+ self.name = var.var.name
if var.dom is None:
self.length = -1
else:
self.length = get_const_int(var.dom.extent)
elif isinstance(var, VirtualAxis):
self.length = var.length
+ if self.name is None:
+ self.name = var.name
Review comment:
In my opinoin, the `name_ct` doesn't really matter because it just makes sure every name is unique, but I am not quite sure about the correct behavior to determine the name either.
@merrymercy could you help clarify?
----------------------------------------------------------------
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] merrymercy edited a comment on pull request #5811: [AutoTVM] Support using IterVar directly in define_reorder with 'candidate' policy.
Posted by GitBox <gi...@apache.org>.
merrymercy edited a comment on pull request #5811:
URL: https://github.com/apache/incubator-tvm/pull/5811#issuecomment-645703625
Users should do the conversion from `IterVar` to `VirtualAxis` by themself in the templates.
Then the equality check follows the default semantics of python, which is check by reference.
Then the `candidates` argument works correctly.
Checking equality by name is not desired. I close this PR for now. Feel free to open if you have better equality check strategies.
----------------------------------------------------------------
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] merrymercy edited a comment on pull request #5811: [AutoTVM][Fix] Fixed 'not in list' error for define_reorder with 'candidate' policy.
Posted by GitBox <gi...@apache.org>.
merrymercy edited a comment on pull request #5811:
URL: https://github.com/apache/incubator-tvm/pull/5811#issuecomment-645563545
In the original design and underlying implementation, all APIs should only accept VirtualAxis.
Later, we make some APIs support IterVar as sugar.
@moderato Please fix the CI.
----------------------------------------------------------------
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] merrymercy edited a comment on pull request #5811: [AutoTVM][Fix] Fixed 'not in list' error for define_reorder with 'candidate' policy.
Posted by GitBox <gi...@apache.org>.
merrymercy edited a comment on pull request #5811:
URL: https://github.com/apache/incubator-tvm/pull/5811#issuecomment-645563545
In the original design and underlying implementation, All APIs should only accept VirtualAxis.
Later, we make some APIs support IterVar as sugar.
Please fix the CI.
----------------------------------------------------------------
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] moderato commented on pull request #5811: [AutoTVM][Fix] Fixed 'not in list' error for define_reorder with 'candidate' policy.
Posted by GitBox <gi...@apache.org>.
moderato commented on pull request #5811:
URL: https://github.com/apache/incubator-tvm/pull/5811#issuecomment-644299685
@comaniac Thank you for the review! I look into the example you mentioned, and I think it works well because the candidates pass to kwargs of the `_add_new_transform` function are lists of `VirtualAxis`, while in my application candidates are lists of `IterVar`. Is `define_reorder` supposed to accept `IterVar`s as arguments?
----------------------------------------------------------------
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] moderato commented on pull request #5811: [AutoTVM][Fix] Fixed 'not in list' error for define_reorder with 'candidate' policy.
Posted by GitBox <gi...@apache.org>.
moderato commented on pull request #5811:
URL: https://github.com/apache/incubator-tvm/pull/5811#issuecomment-645638451
> The `name` is just for printing and debugging. Ideally, you should not use it for the equality check.
> You should only create one VirtualAxis for one IterVar.
Yeah, my question is, when a `VirtualAxis` is being constructed and a name is passed to the constructor, should we assign it to the name of this new `VirtualAxis`? Or we use the name of the `var` if it's a `VirtualAxis` or `IterVar`?
----------------------------------------------------------------
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] merrymercy commented on pull request #5811: [AutoTVM][Fix] Fixed 'not in list' error for define_reorder with 'candidate' policy.
Posted by GitBox <gi...@apache.org>.
merrymercy commented on pull request #5811:
URL: https://github.com/apache/incubator-tvm/pull/5811#issuecomment-645690245
Because the 'name' is only used for printing and debugging. It does not matter.
I am saying the ideal way is that you create VirtualAxis in your templates explicitly and do not reply on `name` to do the equality check.
So we can even cloes this PR.
----------------------------------------------------------------
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] merrymercy edited a comment on pull request #5811: [AutoTVM] Support using IterVar directly in define_reorder with 'candidate' policy.
Posted by GitBox <gi...@apache.org>.
merrymercy edited a comment on pull request #5811:
URL: https://github.com/apache/incubator-tvm/pull/5811#issuecomment-645702581
Users should do the conversion from `IterVar` to `VirtualAxis` by themself in the templates.
Then the equality check follows the default semantics of python, which is check by reference.
Then the `candidates` argument works correctly.
If you are happy with this design, you can close this PR. Otherwise, the priority of which `name` to choose does no matter to me, you can choose either.
----------------------------------------------------------------
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] comaniac commented on a change in pull request #5811: [AutoTVM][Fix] Fixed 'not in list' error for define_reorder with 'candidate' policy.
Posted by GitBox <gi...@apache.org>.
comaniac commented on a change in pull request #5811:
URL: https://github.com/apache/incubator-tvm/pull/5811#discussion_r440313183
##########
File path: python/tvm/autotvm/task/space.py
##########
@@ -119,21 +125,26 @@ def __init__(self, var, name=None):
super(VirtualAxis, self).__init__()
self.num_output = 1
- if name is None:
- name = 'axis_%d' % VirtualAxis.name_ct
- VirtualAxis.name_ct += 1
+ self.name = None
+ if name is not None:
+ self.name = name
- self.name = name
if isinstance(var, (int, _long)):
self.length = var
+ if name is None:
+ self.name = 'axis_%d' % VirtualAxis.name_ct
+ VirtualAxis.name_ct += 1
elif isinstance(var, schedule.IterVar):
- self.name = var.var.name
+ if self.name is None:
+ self.name = var.var.name
if var.dom is None:
self.length = -1
else:
self.length = get_const_int(var.dom.extent)
elif isinstance(var, VirtualAxis):
self.length = var.length
+ if self.name is None:
+ self.name = var.name
Review comment:
It seems doesn't match the original semantic?
##########
File path: python/tvm/autotvm/task/space.py
##########
@@ -119,21 +125,26 @@ def __init__(self, var, name=None):
super(VirtualAxis, self).__init__()
self.num_output = 1
- if name is None:
- name = 'axis_%d' % VirtualAxis.name_ct
- VirtualAxis.name_ct += 1
+ self.name = None
+ if name is not None:
+ self.name = name
Review comment:
You can just use `self.name = name` to cover 3 lines.
##########
File path: python/tvm/autotvm/task/space.py
##########
@@ -119,21 +125,26 @@ def __init__(self, var, name=None):
super(VirtualAxis, self).__init__()
self.num_output = 1
- if name is None:
- name = 'axis_%d' % VirtualAxis.name_ct
- VirtualAxis.name_ct += 1
+ self.name = None
+ if name is not None:
+ self.name = name
- self.name = name
if isinstance(var, (int, _long)):
self.length = var
+ if name is None:
+ self.name = 'axis_%d' % VirtualAxis.name_ct
+ VirtualAxis.name_ct += 1
elif isinstance(var, schedule.IterVar):
- self.name = var.var.name
+ if self.name is None:
+ self.name = var.var.name
if var.dom is None:
self.length = -1
else:
self.length = get_const_int(var.dom.extent)
elif isinstance(var, VirtualAxis):
Review comment:
Modify the class docstring (`var: int or tvm.te.schedule.IterVar`) to add `VirtualAxis`.
----------------------------------------------------------------
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