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/11 16:54:50 UTC
[GitHub] [tvm] csullivan opened a new pull request #8017: [IR] Add storage scope to PointerType
csullivan opened a new pull request #8017:
URL: https://github.com/apache/tvm/pull/8017
Add storage scope into which a PointerType may address. Scope is global by default. The storage scope field can allow for specialization during lowering and codegen (e.g. when the kernel function signature is storage scope dependent during codegen). This satisfies the need in #7686 for a pointer to texture memory.
--
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] tqchen commented on pull request #8017: [IR] Add storage scope to PointerType
Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #8017:
URL: https://github.com/apache/tvm/pull/8017#issuecomment-838853629
cc @vinx13 @Hzfengsy @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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] mbaret commented on pull request #8017: [IR] Add storage scope to PointerType
Posted by GitBox <gi...@apache.org>.
mbaret commented on pull request #8017:
URL: https://github.com/apache/tvm/pull/8017#issuecomment-842178152
This looks to be in competition with the `attr::storage_scope` attribute used for Allocate statements. Could you elaborate a bit on why we need this new mechanism in addition to the existing one? I had a look at the discussion on https://github.com/apache/tvm/pull/7686 but it wasn't obvious to me.
--
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] tqchen commented on pull request #8017: [IR] Add storage scope to PointerType
Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #8017:
URL: https://github.com/apache/tvm/pull/8017#issuecomment-843171131
Thanks @csullivan @Hzfengsy @mbaret
--
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] tqchen commented on pull request #8017: [IR] Add storage scope to PointerType
Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #8017:
URL: https://github.com/apache/tvm/pull/8017#issuecomment-842554547
@mbaret The are related. The scope information is only availiable in the alloca stmt, while the scope in pointer type can propagate as we pass the values, which can help our analysis by a lot. In the alloca stmt itself this might results in a duplicated information, as a next step we can update, and ensure that the pointer type's scope is consistent with the scope in alloca so there won't be inconsistency issue.
We have done the similar thing for dtype field of alloca and pointers, and I believe we should do this as well as our next steps.
--
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] Hzfengsy commented on a change in pull request #8017: [IR] Add storage scope to PointerType
Posted by GitBox <gi...@apache.org>.
Hzfengsy commented on a change in pull request #8017:
URL: https://github.com/apache/tvm/pull/8017#discussion_r630691285
##########
File path: src/ir/type.cc
##########
@@ -43,21 +43,26 @@ TVM_STATIC_IR_FUNCTOR(ReprPrinter, vtable)
p->stream << node->dtype;
});
-PointerType::PointerType(Type element_type) {
+PointerType::PointerType(Type element_type, String storage_scope) {
ObjectPtr<PointerTypeNode> n = make_object<PointerTypeNode>();
n->element_type = std::move(element_type);
+ n->storage_scope = std::move(storage_scope);
data_ = std::move(n);
}
TVM_REGISTER_NODE_TYPE(PointerTypeNode);
-TVM_REGISTER_GLOBAL("ir.PointerType").set_body_typed([](Type element_type) {
- return PointerType(element_type);
-});
+TVM_REGISTER_GLOBAL("ir.PointerType")
+ .set_body_typed([](Type element_type, String storage_scope = "") {
+ return PointerType(element_type, storage_scope);
+ });
TVM_STATIC_IR_FUNCTOR(ReprPrinter, vtable)
.set_dispatch<PointerTypeNode>([](const ObjectRef& ref, ReprPrinter* p) {
auto* node = static_cast<const PointerTypeNode*>(ref.get());
+ if (node->storage_scope.size()) {
Review comment:
```suggestion
if (!node->storage_scope.empty()) {
```
##########
File path: src/printer/tvmscript_printer.cc
##########
@@ -764,7 +764,11 @@ Doc TVMScriptPrinter::VisitType_(const PrimTypeNode* node) {
Doc TVMScriptPrinter::VisitType_(const PointerTypeNode* node) {
Doc doc;
- doc << "ty.Ptr[" << Print(node->element_type) << "]";
+ doc << "ty.Ptr[";
+ if (node->storage_scope.size()) {
Review comment:
```suggestion
if (!node->storage_scope.empty()) {
```
##########
File path: src/printer/tir_text_printer.cc
##########
@@ -620,7 +620,11 @@ Doc TIRTextPrinter::VisitType_(const PrimTypeNode* node) {
Doc TIRTextPrinter::VisitType_(const PointerTypeNode* node) {
Doc doc;
- doc << "Pointer(" << Print(node->element_type) << ")";
+ doc << "Pointer(";
+ if (node->storage_scope.size()) {
Review comment:
```suggestion
if (!node->storage_scope.empty()) {
```
--
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] tqchen commented on a change in pull request #8017: [IR] Add storage scope to PointerType
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #8017:
URL: https://github.com/apache/tvm/pull/8017#discussion_r630374761
##########
File path: python/tvm/ir/type.py
##########
@@ -73,8 +73,8 @@ class PointerType(Type):
The type of pointer's element.
"""
- def __init__(self, element_type):
- self.__init_handle_by_constructor__(_ffi_api.PointerType, element_type)
+ def __init__(self, element_type, storage_scope="global"):
Review comment:
Update the docsstring above
--
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] mbaret commented on pull request #8017: [IR] Add storage scope to PointerType
Posted by GitBox <gi...@apache.org>.
mbaret commented on pull request #8017:
URL: https://github.com/apache/tvm/pull/8017#issuecomment-842178152
This looks to be in competition with the `attr::storage_scope` attribute used for Allocate statements. Could you elaborate a bit on why we need this new mechanism in addition to the existing one? I had a look at the discussion on https://github.com/apache/tvm/pull/7686 but it wasn't obvious to me.
--
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] csullivan commented on pull request #8017: [IR] Add storage scope to PointerType
Posted by GitBox <gi...@apache.org>.
csullivan commented on pull request #8017:
URL: https://github.com/apache/tvm/pull/8017#issuecomment-842523174
Good question, the Type subclasses can be used as a type_annotation to [tir variables](https://github.com/csullivan/incubator-tvm/blob/main/include/tvm/tir/var.h#L61). Code generation specifically relies on this type annotation to correctly write an op's function signature and the accesses of that Var.
Taking #7686 as an example, when generating a function signature for `var` s.t.,
```
var = Var("a_buffer_var", PointerType(PrimType("float16"), "global.texture"))
```
we expect PrintType(var) to return `image2d_t` in OpenCL. In Metal this may be `texture2d<half,>`.
That's not to say we couldn't use the AST annotation to annotate Vars, but this would require all lowering to respect the fact that Vars may have annotations and thus to carry them forward if any Var binding occurs. OTOH if the storage_scope is part of the type system it is guaranteed to be preserved.
--
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] tqchen merged pull request #8017: [IR] Add storage scope to PointerType
Posted by GitBox <gi...@apache.org>.
tqchen merged pull request #8017:
URL: https://github.com/apache/tvm/pull/8017
--
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] tqchen commented on a change in pull request #8017: [IR] Add storage scope to PointerType
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #8017:
URL: https://github.com/apache/tvm/pull/8017#discussion_r630374276
##########
File path: include/tvm/ir/type.h
##########
@@ -175,8 +182,9 @@ class PointerType : public Type {
/*!
* \brief Constructor
* \param element_type The type of the element which the pointer points to.
+ * \param storage_scope The storage scope into which the pointer addresses
*/
- TVM_DLL explicit PointerType(Type element_type);
+ TVM_DLL explicit PointerType(Type element_type, String storage_scope = "global");
Review comment:
To be consistent with the rest of the code base, let us still allow empty string, which indicates that it is global scope. We do not need to print out the scope of global ptr to simplify the readability of the Ptr type.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org