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 2021/05/28 17:59:52 UTC
[GitHub] [tvm] Lunderberg opened a new pull request #8161: [VM] Avoid round-trip Target->str->Target conversions
Lunderberg opened a new pull request #8161:
URL: https://github.com/apache/tvm/pull/8161
Currently, in some cases this round-trip cannot be completed. For example, if an Integer value has a value outside a 32-bit signed integer range, or if a String value contains spaces.
--
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] [tvm] comaniac commented on a change in pull request #8161: [VM] Avoid round-trip Target->str->Target conversions
Posted by GitBox <gi...@apache.org>.
comaniac commented on a change in pull request #8161:
URL: https://github.com/apache/tvm/pull/8161#discussion_r641731595
##########
File path: python/tvm/relay/backend/vm.py
##########
@@ -198,20 +198,29 @@ def _update_target(self, target):
target = target if target else tvm.target.Target.current()
if target is None:
raise ValueError("Target is not set in env or passed as argument.")
- tgts = {}
- if isinstance(target, (str, tvm.target.Target)):
- dev_type = tvm.tir.IntImm("int32", tvm.nd.device(str(target)).device_type)
- tgts[dev_type] = tvm.target.Target(target)
- elif isinstance(target, dict):
- for dev, tgt in target.items():
- dev_type = tvm.tir.IntImm("int32", tvm.nd.device(dev).device_type)
- tgts[dev_type] = tvm.target.Target(tgt)
- else:
+
+ elif isinstance(target, str):
+ target = {target: target}
+
+ elif isinstance(target, tvm.target.Target):
+ target = {target.kind.name: target}
+
Review comment:
remove this line
##########
File path: python/tvm/relay/backend/vm.py
##########
@@ -198,20 +198,29 @@ def _update_target(self, target):
target = target if target else tvm.target.Target.current()
if target is None:
raise ValueError("Target is not set in env or passed as argument.")
- tgts = {}
- if isinstance(target, (str, tvm.target.Target)):
- dev_type = tvm.tir.IntImm("int32", tvm.nd.device(str(target)).device_type)
- tgts[dev_type] = tvm.target.Target(target)
- elif isinstance(target, dict):
- for dev, tgt in target.items():
- dev_type = tvm.tir.IntImm("int32", tvm.nd.device(dev).device_type)
- tgts[dev_type] = tvm.target.Target(tgt)
- else:
+
+ elif isinstance(target, str):
+ target = {target: target}
+
Review comment:
remove this line
##########
File path: python/tvm/relay/backend/vm.py
##########
@@ -198,20 +198,29 @@ def _update_target(self, target):
target = target if target else tvm.target.Target.current()
if target is None:
raise ValueError("Target is not set in env or passed as argument.")
- tgts = {}
- if isinstance(target, (str, tvm.target.Target)):
- dev_type = tvm.tir.IntImm("int32", tvm.nd.device(str(target)).device_type)
- tgts[dev_type] = tvm.target.Target(target)
- elif isinstance(target, dict):
- for dev, tgt in target.items():
- dev_type = tvm.tir.IntImm("int32", tvm.nd.device(dev).device_type)
- tgts[dev_type] = tvm.target.Target(tgt)
- else:
+
+ elif isinstance(target, str):
+ target = {target: target}
+
+ elif isinstance(target, tvm.target.Target):
+ target = {target.kind.name: target}
+
+ elif not isinstance(target, dict):
raise TypeError(
"target is expected to be str, tvm.target.Target, "
+ "or dict of str to str/tvm.target.Target, but received "
+ "{}".format(type(target))
)
+
+ tgts = {}
+
Review comment:
remove this line
--
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] [tvm] comaniac commented on pull request #8161: [VM] Avoid round-trip Target->str->Target conversions
Posted by GitBox <gi...@apache.org>.
comaniac commented on pull request #8161:
URL: https://github.com/apache/tvm/pull/8161#issuecomment-850733401
> LGTM, thanks for fixing this corner case on rountrip conversion @Lunderberg . I think @comaniac 's comments are on formatting, if I'm reading this correctly.
Yes it was about formatting.
--
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] [tvm] Lunderberg commented on a change in pull request #8161: [VM] Avoid round-trip Target->str->Target conversions
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on a change in pull request #8161:
URL: https://github.com/apache/tvm/pull/8161#discussion_r641832303
##########
File path: python/tvm/relay/backend/vm.py
##########
@@ -198,20 +198,29 @@ def _update_target(self, target):
target = target if target else tvm.target.Target.current()
if target is None:
raise ValueError("Target is not set in env or passed as argument.")
- tgts = {}
- if isinstance(target, (str, tvm.target.Target)):
- dev_type = tvm.tir.IntImm("int32", tvm.nd.device(str(target)).device_type)
- tgts[dev_type] = tvm.target.Target(target)
- elif isinstance(target, dict):
- for dev, tgt in target.items():
- dev_type = tvm.tir.IntImm("int32", tvm.nd.device(dev).device_type)
- tgts[dev_type] = tvm.target.Target(tgt)
- else:
+
+ elif isinstance(target, str):
+ target = {target: target}
+
Review comment:
I think this line should be present, in order to maintain previous behavior. The `target` variable comes from either `VMCompiler.lower` or `VMCompiler.optimize`, both of which state that the `target` parameter can be a string, a `tvm.target.Target`, or a dictionary. If this line were removed, then there would be a type error later on when `target` is accessed as a dictionary, if a `str` argument is passed in.
The modified version moves all the input-handling into a single location, making a dictionary regardless of the input value, so that the remainder of the function doesn't need to repeat the logic for how to set the output `tgts` dictionary.
##########
File path: python/tvm/relay/backend/vm.py
##########
@@ -198,20 +198,29 @@ def _update_target(self, target):
target = target if target else tvm.target.Target.current()
if target is None:
raise ValueError("Target is not set in env or passed as argument.")
- tgts = {}
- if isinstance(target, (str, tvm.target.Target)):
- dev_type = tvm.tir.IntImm("int32", tvm.nd.device(str(target)).device_type)
- tgts[dev_type] = tvm.target.Target(target)
- elif isinstance(target, dict):
- for dev, tgt in target.items():
- dev_type = tvm.tir.IntImm("int32", tvm.nd.device(dev).device_type)
- tgts[dev_type] = tvm.target.Target(tgt)
- else:
+
+ elif isinstance(target, str):
+ target = {target: target}
+
+ elif isinstance(target, tvm.target.Target):
+ target = {target.kind.name: target}
+
Review comment:
Same comment as for the previous, but if a `tvm.target.Target` is passed to `VMCompiler.lower`.
--
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] [tvm] Lunderberg commented on pull request #8161: [VM] Avoid round-trip Target->str->Target conversions
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on pull request #8161:
URL: https://github.com/apache/tvm/pull/8161#issuecomment-850695179
Thank you! A couple comments on the nits, and pushed an update to fix a lint error.
--
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] [tvm] masahi merged pull request #8161: [VM] Avoid round-trip Target->str->Target conversions
Posted by GitBox <gi...@apache.org>.
masahi merged pull request #8161:
URL: https://github.com/apache/tvm/pull/8161
--
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] [tvm] Lunderberg commented on pull request #8161: [VM] Avoid round-trip Target->str->Target conversions
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on pull request #8161:
URL: https://github.com/apache/tvm/pull/8161#issuecomment-850733570
Thank you, both, and that makes a lot more sense than my initial interpretation. Whitespace should be fixed up 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] [tvm] Lunderberg commented on a change in pull request #8161: [VM] Avoid round-trip Target->str->Target conversions
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on a change in pull request #8161:
URL: https://github.com/apache/tvm/pull/8161#discussion_r641832386
##########
File path: python/tvm/relay/backend/vm.py
##########
@@ -198,20 +198,29 @@ def _update_target(self, target):
target = target if target else tvm.target.Target.current()
if target is None:
raise ValueError("Target is not set in env or passed as argument.")
- tgts = {}
- if isinstance(target, (str, tvm.target.Target)):
- dev_type = tvm.tir.IntImm("int32", tvm.nd.device(str(target)).device_type)
- tgts[dev_type] = tvm.target.Target(target)
- elif isinstance(target, dict):
- for dev, tgt in target.items():
- dev_type = tvm.tir.IntImm("int32", tvm.nd.device(dev).device_type)
- tgts[dev_type] = tvm.target.Target(tgt)
- else:
+
+ elif isinstance(target, str):
+ target = {target: target}
+
+ elif isinstance(target, tvm.target.Target):
+ target = {target.kind.name: target}
+
+ elif not isinstance(target, dict):
raise TypeError(
"target is expected to be str, tvm.target.Target, "
+ "or dict of str to str/tvm.target.Target, but received "
+ "{}".format(type(target))
)
+
+ tgts = {}
+
Review comment:
I think this line necessary, because this is the return value into which the results are collected.
--
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