Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[IR] Add storage scope to PointerType #8017

Merged
merged 2 commits into from
May 18, 2021
Merged

[IR] Add storage scope to PointerType #8017

merged 2 commits into from
May 18, 2021

Conversation

csullivan
Copy link
Contributor

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.

include/tvm/ir/type.h Outdated Show resolved Hide resolved
python/tvm/ir/type.py Outdated Show resolved Hide resolved
@tqchen
Copy link
Member

tqchen commented May 11, 2021

src/printer/tir_text_printer.cc Outdated Show resolved Hide resolved
src/ir/type.cc Outdated Show resolved Hide resolved
src/printer/tvmscript_printer.cc Outdated Show resolved Hide resolved
@tqchen tqchen added the status: need update need update based on feedbacks label May 13, 2021
@tqchen tqchen self-assigned this May 13, 2021
@mbaret
Copy link
Contributor

mbaret commented May 17, 2021

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 #7686 but it wasn't obvious to me.

Co-authored-by: Siyuan Feng <Hzfengsy@sjtu.edu.cn>
@csullivan
Copy link
Contributor Author

Good question, the Type subclasses can be used as a type_annotation to tir variables. 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.

@tqchen
Copy link
Member

tqchen commented May 17, 2021

@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.

@tqchen tqchen merged commit c510c2b into apache:main May 18, 2021
@tqchen
Copy link
Member

tqchen commented May 18, 2021

Thanks @csullivan @Hzfengsy @mbaret

@tqchen tqchen added status: accepted and removed status: need update need update based on feedbacks labels May 18, 2021
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Jun 17, 2021
* Add storage scope to PointerType.

* Apply suggestions from code review

Co-authored-by: Siyuan Feng <Hzfengsy@sjtu.edu.cn>
trevor-m pushed a commit to neo-ai/tvm that referenced this pull request Jun 17, 2021
* Add storage scope to PointerType.

* Apply suggestions from code review

Co-authored-by: Siyuan Feng <Hzfengsy@sjtu.edu.cn>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants