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 2022/01/28 18:04:10 UTC
[GitHub] [tvm] Lunderberg opened a new pull request #10099: [TVMScript] Support T.buffer_decl using data pointer from Let/Allocate
Lunderberg opened a new pull request #10099:
URL: https://github.com/apache/tvm/pull/10099
Supporting this use case required additional support on both the parsing side and the tvmscript printing side.
* Parse pointer objects `T.Ptr[T.int32]` in TVMScript. Previously, there was no handling of `ast.TypeApply` in `TVMScriptParser`.
* Call `T.buffer_decl` in the function body if the data variable is defined by Let/Allocate. Previously, these buffer declarations were hoisted to the function header, prior to the declaration of the data variable they require.
These constructions come up in multiple cases in the updated unit tests for #9727, typically in round-trip unit tests for PrimFuncs that have been passed through `MakePackedAPI`. This change does not depend on the changes included in #9727, and is therefore kept separate.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Lunderberg commented on a change in pull request #10099: [TVMScript] Support T.buffer_decl using data pointer from Let/Allocate
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on a change in pull request #10099:
URL: https://github.com/apache/tvm/pull/10099#discussion_r798653970
##########
File path: python/tvm/script/parser.py
##########
@@ -1138,6 +1141,29 @@ def transform_TypeTuple(self, node):
"""
return [self.transform(value) for value in node.values]
+ def transform_TypeApply(self, node):
+ """Visitor for Type[Type] expressions.
+
+ Mostly used for ``T.Ptr`` expressions.
+ """
+ func = self.transform(node.func_name)
+
+ if not isinstance(func, ty.TypeGeneric):
+ self.report_error(f"Expected a type but found {type(func).__name__}", node.span)
+
+ param_types = []
Review comment:
I had looked at the `transform_TypeTuple` implementation, and I don't think it's directly applicable. It assumes that there is a `synr.ast.TypeTuple` node, with types in `node.values`, and doesn't have a way to accept a list of types directly or to access the `node.params` of a `TypeApply` node. The `TVMScriptParser.parse_arg_list` is the closest I found to the desired functionality, but it requires the node to be an intrinsic, scope handler, or special statement, and doesn't have a case for type annotations.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Lunderberg commented on a change in pull request #10099: [TVMScript] Support T.buffer_decl using data pointer from Let/Allocate
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on a change in pull request #10099:
URL: https://github.com/apache/tvm/pull/10099#discussion_r798615771
##########
File path: tests/python/unittest/test_tvmscript_roundtrip.py
##########
@@ -3255,5 +3255,44 @@ def test_root_attr():
tvm.ir.assert_structural_equal(func, rt_func, True)
+@T.prim_func
+def func_T_ptr_let_statement(
+ args: T.handle, arg_type_ids_handle: T.Ptr[T.int32], num_args: T.int32
+) -> None:
+ # buffer definition
+ arg_type_ids = T.buffer_decl([2], dtype="int32", data=arg_type_ids_handle)
+ # body
+ assert num_args == 2, "main: num_args should be 3"
Review comment:
Thank you, and the comment is incorrect. The check here isn't strictly necessary, and was included to mimic the functions generated by `MakePackedAPI`. I've removed the assert, and added comments to explicitly state the behavior being tested.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Lunderberg commented on a change in pull request #10099: [TVMScript] Support T.buffer_decl using data pointer from Let/Allocate
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on a change in pull request #10099:
URL: https://github.com/apache/tvm/pull/10099#discussion_r798645648
##########
File path: python/tvm/script/parser.py
##########
@@ -1138,6 +1141,29 @@ def transform_TypeTuple(self, node):
"""
return [self.transform(value) for value in node.values]
+ def transform_TypeApply(self, node):
+ """Visitor for Type[Type] expressions.
+
+ Mostly used for ``T.Ptr`` expressions.
+ """
+ func = self.transform(node.func_name)
+
+ if not isinstance(func, ty.TypeGeneric):
+ self.report_error(f"Expected a type but found {type(func).__name__}", node.span)
+
+ param_types = []
+ for param in node.params:
+ param_type = self.transform(param)
+ if not isinstance(param_type, ty.TypeGeneric):
Review comment:
It doesn't, no. The parameters of `TypeApply` typically aren't `GenericPtrType`. For example, `T.Ptr[T.int32]`, the `T.int32` parameter is a `ConcreteType`.
At some point, I may see if there's support for renaming `ty.TypeGeneric` to `ty.Type`. As it is, `ty.ConcreteType` is a subclass of `ty.TypeGeneric`, which doesn't make very much sense to me as there since it require any generic parameters in the user-supplied tvmscript.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao1994 commented on pull request #10099: [TVMScript] Support T.buffer_decl using data pointer from Let/Allocate
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on pull request #10099:
URL: https://github.com/apache/tvm/pull/10099#issuecomment-1024638501
CC: @Hzfengsy @vinx13 @spectrometerHBH
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Lunderberg commented on a change in pull request #10099: [TVMScript] Support T.buffer_decl using data pointer from Let/Allocate
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on a change in pull request #10099:
URL: https://github.com/apache/tvm/pull/10099#discussion_r798637887
##########
File path: python/tvm/script/parser.py
##########
@@ -1138,6 +1141,29 @@ def transform_TypeTuple(self, node):
"""
return [self.transform(value) for value in node.values]
+ def transform_TypeApply(self, node):
+ """Visitor for Type[Type] expressions.
+
+ Mostly used for ``T.Ptr`` expressions.
+ """
+ func = self.transform(node.func_name)
+
+ if not isinstance(func, ty.TypeGeneric):
+ self.report_error(f"Expected a type but found {type(func).__name__}", node.span)
Review comment:
Similar to above, I'd like to avoid breaking any usage of `T.Tuple`, if it exists outside of the implementation above. I've updated the error message to read `"Use of type arguments requires a type that accepts type arguments (e.g. T.Ptr), but found {type(func).__name__} instead."` to clarify that the error would be in applying a type argument, not in the use of a type annotation.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Lunderberg commented on a change in pull request #10099: [TVMScript] Support T.buffer_decl using data pointer from Let/Allocate
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on a change in pull request #10099:
URL: https://github.com/apache/tvm/pull/10099#discussion_r798630222
##########
File path: python/tvm/script/parser.py
##########
@@ -1138,6 +1141,29 @@ def transform_TypeTuple(self, node):
"""
return [self.transform(value) for value in node.values]
+ def transform_TypeApply(self, node):
+ """Visitor for Type[Type] expressions.
+
+ Mostly used for ``T.Ptr`` expressions.
+ """
+ func = self.transform(node.func_name)
+
+ if not isinstance(func, ty.TypeGeneric):
Review comment:
From my understanding, the `TypeApply` ast node would also be used for `T.Tuple[type1,type2]` type annotations. While I couldn't find any usage of `T.Tuple` type annotations, `GenericTupleType` exists in `tvm.script.tir.ty` and would also use `TypeApply` in the synr ast. I've added a check for `hasattr(func, __getitem__)`, since that expresses the intent that this is a type that can accept type arguments, and would work for both `T.Ptr` and `T.Tuple`.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Lunderberg commented on a change in pull request #10099: [TVMScript] Support T.buffer_decl using data pointer from Let/Allocate
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on a change in pull request #10099:
URL: https://github.com/apache/tvm/pull/10099#discussion_r798637887
##########
File path: python/tvm/script/parser.py
##########
@@ -1138,6 +1141,29 @@ def transform_TypeTuple(self, node):
"""
return [self.transform(value) for value in node.values]
+ def transform_TypeApply(self, node):
+ """Visitor for Type[Type] expressions.
+
+ Mostly used for ``T.Ptr`` expressions.
+ """
+ func = self.transform(node.func_name)
+
+ if not isinstance(func, ty.TypeGeneric):
+ self.report_error(f"Expected a type but found {type(func).__name__}", node.span)
Review comment:
Similar to above, I'd like to avoid breaking any usage of `T.Tuple`, if it exists outside of the implementation above. I've updated the error message to read `"Use of type arguments requires a type that accepts type arguments (e.g. T.Ptr), but found {type(func).__name__} instead."` to clarify that the error would be in applying a type argument, not in the type annotation itself.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] shingjan commented on a change in pull request #10099: [TVMScript] Support T.buffer_decl using data pointer from Let/Allocate
Posted by GitBox <gi...@apache.org>.
shingjan commented on a change in pull request #10099:
URL: https://github.com/apache/tvm/pull/10099#discussion_r798134591
##########
File path: python/tvm/script/parser.py
##########
@@ -1138,6 +1141,29 @@ def transform_TypeTuple(self, node):
"""
return [self.transform(value) for value in node.values]
+ def transform_TypeApply(self, node):
+ """Visitor for Type[Type] expressions.
+
+ Mostly used for ``T.Ptr`` expressions.
+ """
+ func = self.transform(node.func_name)
+
+ if not isinstance(func, ty.TypeGeneric):
+ self.report_error(f"Expected a type but found {type(func).__name__}", node.span)
Review comment:
`Expect a type` -> `Expect a GenericPtrType` maybe be better
##########
File path: python/tvm/script/parser.py
##########
@@ -1138,6 +1141,29 @@ def transform_TypeTuple(self, node):
"""
return [self.transform(value) for value in node.values]
+ def transform_TypeApply(self, node):
+ """Visitor for Type[Type] expressions.
+
+ Mostly used for ``T.Ptr`` expressions.
+ """
+ func = self.transform(node.func_name)
+
+ if not isinstance(func, ty.TypeGeneric):
+ self.report_error(f"Expected a type but found {type(func).__name__}", node.span)
+
+ param_types = []
Review comment:
there is a `transform_TypeTuple` impl that may be useful here
##########
File path: python/tvm/script/tir/ty.py
##########
@@ -65,6 +70,8 @@ class GenericTupleType(TypeGeneric): # pylint: disable=abstract-method
"""
def __getitem__(self, vtypes):
+ if isinstance(vtypes, TypeGeneric):
Review comment:
This seems to be a bug fix right?
##########
File path: tests/python/unittest/test_tvmscript_roundtrip.py
##########
@@ -3255,5 +3255,44 @@ def test_root_attr():
tvm.ir.assert_structural_equal(func, rt_func, True)
+@T.prim_func
+def func_T_ptr_let_statement(
+ args: T.handle, arg_type_ids_handle: T.Ptr[T.int32], num_args: T.int32
+) -> None:
+ # buffer definition
+ arg_type_ids = T.buffer_decl([2], dtype="int32", data=arg_type_ids_handle)
+ # body
+ assert num_args == 2, "main: num_args should be 3"
Review comment:
mismatch between the code and comment here. BTW is this check necessary here?
##########
File path: python/tvm/script/parser.py
##########
@@ -1138,6 +1141,29 @@ def transform_TypeTuple(self, node):
"""
return [self.transform(value) for value in node.values]
+ def transform_TypeApply(self, node):
+ """Visitor for Type[Type] expressions.
+
+ Mostly used for ``T.Ptr`` expressions.
+ """
+ func = self.transform(node.func_name)
+
+ if not isinstance(func, ty.TypeGeneric):
+ self.report_error(f"Expected a type but found {type(func).__name__}", node.span)
+
+ param_types = []
+ for param in node.params:
+ param_type = self.transform(param)
+ if not isinstance(param_type, ty.TypeGeneric):
Review comment:
Does this for-loop imply that all params of `TypeApply` call will be transformed into `GenericPtrType`? If so can we specify that here as well instead of `ty.TypeGeneric`
##########
File path: python/tvm/script/parser.py
##########
@@ -1138,6 +1141,29 @@ def transform_TypeTuple(self, node):
"""
return [self.transform(value) for value in node.values]
+ def transform_TypeApply(self, node):
+ """Visitor for Type[Type] expressions.
+
+ Mostly used for ``T.Ptr`` expressions.
+ """
+ func = self.transform(node.func_name)
+
+ if not isinstance(func, ty.TypeGeneric):
Review comment:
Use of `GenericPtrType` here maybe more accurate
##########
File path: python/tvm/script/tir/ty.py
##########
@@ -44,6 +44,9 @@ def __init__(self, vtype):
self.type = vtype
def evaluate(self):
+ if isinstance(self.type, tvm.ir.Type):
Review comment:
I thought this change only impact `GenericPtrType` instead of `ConcreteType` here. Is that the case?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tmoreau89 merged pull request #10099: [TVMScript] Support T.buffer_decl using data pointer from Let/Allocate
Posted by GitBox <gi...@apache.org>.
tmoreau89 merged pull request #10099:
URL: https://github.com/apache/tvm/pull/10099
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Lunderberg commented on a change in pull request #10099: [TVMScript] Support T.buffer_decl using data pointer from Let/Allocate
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on a change in pull request #10099:
URL: https://github.com/apache/tvm/pull/10099#discussion_r798670765
##########
File path: python/tvm/script/tir/ty.py
##########
@@ -44,6 +44,9 @@ def __init__(self, vtype):
self.type = vtype
def evaluate(self):
+ if isinstance(self.type, tvm.ir.Type):
Review comment:
This check gets hit when calling `.evaluate()` on the return type of `GenericPtrType.__getitem__`. The return type is a `ConcreteType` that has been passed a `tvm.ir.PointerType`, which should then be returned directly when the concrete type is evaluated, rather than the usual behavior of wrapping it in a `tvm.ir.PrimType`.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Lunderberg commented on a change in pull request #10099: [TVMScript] Support T.buffer_decl using data pointer from Let/Allocate
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on a change in pull request #10099:
URL: https://github.com/apache/tvm/pull/10099#discussion_r798665516
##########
File path: python/tvm/script/tir/ty.py
##########
@@ -65,6 +70,8 @@ class GenericTupleType(TypeGeneric): # pylint: disable=abstract-method
"""
def __getitem__(self, vtypes):
+ if isinstance(vtypes, TypeGeneric):
Review comment:
That's correct. If `transform_TypeApply` was to be able to handle both `T.Ptr` and `T.Tuple` cases, then I wanted to make sure that they both accepted the same types of arguments. The two options I considered were (a) always passing a tuple of parameter types even if there is only 1, or (b) passing a bare type when there is only 1 and otherwise passing a tuple of parameter types. Previously, `T.Ptr` implicitly followed convention (b), while `T.Tuple` followed convention (a). Option (b) matches python's subscripting syntax, so that's the one that I chose.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Lunderberg commented on a change in pull request #10099: [TVMScript] Support T.buffer_decl using data pointer from Let/Allocate
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on a change in pull request #10099:
URL: https://github.com/apache/tvm/pull/10099#discussion_r798630222
##########
File path: python/tvm/script/parser.py
##########
@@ -1138,6 +1141,29 @@ def transform_TypeTuple(self, node):
"""
return [self.transform(value) for value in node.values]
+ def transform_TypeApply(self, node):
+ """Visitor for Type[Type] expressions.
+
+ Mostly used for ``T.Ptr`` expressions.
+ """
+ func = self.transform(node.func_name)
+
+ if not isinstance(func, ty.TypeGeneric):
Review comment:
The `TypeApply` functionality also occurs with the `T.Tuple[type1,type2]` type annotation. While I couldn't find any usage of `T.Tuple` type annotations, `GenericTupleType` exists in `tvm.script.tir.ty` and would also use `TypeApply` in the synr ast. I've added a check for `hasattr(func, __getitem__)`, since that expresses the intent that this is a type that can accept type arguments, and would work for both `T.Ptr` and `T.Tuple`.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Lunderberg commented on a change in pull request #10099: [TVMScript] Support T.buffer_decl using data pointer from Let/Allocate
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on a change in pull request #10099:
URL: https://github.com/apache/tvm/pull/10099#discussion_r798677322
##########
File path: python/tvm/script/tir/ty.py
##########
@@ -44,6 +44,9 @@ def __init__(self, vtype):
self.type = vtype
def evaluate(self):
+ if isinstance(self.type, tvm.ir.Type):
Review comment:
After thinking on it, I think that it would be cleaner if the type conversion happens during the call to `__init__`, rather than being delayed until the `.evaluate()` call. That way, the `self.type` object has a consistent type, regardless of whether it was initialized with a string to represent a primitive, or was initialized with a `tvm.ir.Type` to represent that type.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tmoreau89 commented on pull request #10099: [TVMScript] Support T.buffer_decl using data pointer from Let/Allocate
Posted by GitBox <gi...@apache.org>.
tmoreau89 commented on pull request #10099:
URL: https://github.com/apache/tvm/pull/10099#issuecomment-1029354592
Thank you @Lunderberg @shingjan @Hzfengsy - 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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Lunderberg commented on pull request #10099: [TVMScript] Support T.buffer_decl using data pointer from Let/Allocate
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on pull request #10099:
URL: https://github.com/apache/tvm/pull/10099#issuecomment-1024496920
Updated to resolve lint errors.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org