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/01/05 21:45:20 UTC
[GitHub] [tvm] tqchen opened a new pull request #7216: [TIR][REFACTOR] Enforce allocate to use the correct var hint.
tqchen opened a new pull request #7216:
URL: https://github.com/apache/tvm/pull/7216
This is a refactoring step to cleanup legacy issue of opaque buffer
var without type information. Now all the allocation comes with the right
pointer data type. Places touched:
- TVMScript Parser: add the right info to get the correct pointer type.
- Cross thread all reduce: set the right pointer type.
- Storage rewrite: setup the right pointer type.
- Custom dtype: remap the variables with new pointer 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
[GitHub] [tvm] tkonolige commented on a change in pull request #7216: [TIR][REFACTOR] Enforce allocate to use the correct var pointer hint.
Posted by GitBox <gi...@apache.org>.
tkonolige commented on a change in pull request #7216:
URL: https://github.com/apache/tvm/pull/7216#discussion_r552756003
##########
File path: python/tvm/tir/buffer.py
##########
@@ -247,7 +247,9 @@ def decl_buffer(
shape_dtype = shape[0].dtype if hasattr(shape[0], "dtype") else "int32"
elem_offset = Var("%s_elem_offset" % name, shape_dtype)
if data is None:
- data = Var(name, PointerType(PrimType(dtype)), span)
+ # store bool as i8
+ storage_dtype = "int8" if dtype == "bool" else dtype
Review comment:
Could we just have the `PrimType` constructor do the conversion?
----------------------------------------------------------------
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] tkonolige commented on a change in pull request #7216: [TIR][REFACTOR] Enforce allocate to use the correct var pointer hint.
Posted by GitBox <gi...@apache.org>.
tkonolige commented on a change in pull request #7216:
URL: https://github.com/apache/tvm/pull/7216#discussion_r552228734
##########
File path: python/tvm/tir/buffer.py
##########
@@ -247,7 +247,9 @@ def decl_buffer(
shape_dtype = shape[0].dtype if hasattr(shape[0], "dtype") else "int32"
elem_offset = Var("%s_elem_offset" % name, shape_dtype)
if data is None:
- data = Var(name, PointerType(PrimType(dtype)), span)
+ # store bool as i8
+ storage_dtype = "int8" if dtype == "bool" else dtype
Review comment:
Why is there a special case for bool here?
##########
File path: src/target/source/codegen_cuda.cc
##########
@@ -581,7 +581,10 @@ void CodeGenCUDA::VisitStmt_(const AllocateNode* op) {
int32_t constant_size = op->constant_allocation_size();
ICHECK_GT(constant_size, 0) << "Can only handle constant size stack allocation for now";
const VarNode* buffer = op->buffer_var.as<VarNode>();
- std::string scope = alloc_storage_scope_.at(buffer);
+ auto it = alloc_storage_scope_.find(buffer);
+ ICHECK(it != alloc_storage_scope_.end())
Review comment:
I think we could be a little more specific here. We need an `AttrStmt` with a key of `storage_scope` right? So maybe say `"Buffer " << op->buffer_var << " is missing an AttrStmt with a \"storage_scope\" key"`.
##########
File path: src/tir/ir/stmt.cc
##########
@@ -274,9 +274,10 @@ TVM_STATIC_IR_FUNCTOR(ReprPrinter, vtable)
// Allocate
Allocate::Allocate(Var buffer_var, DataType dtype, Array<PrimExpr> extents, PrimExpr condition,
Stmt body, Span span) {
- // TODO(tvm-team): Add invariant check to make sure
- // IsPointerPType(buffer_var->type_annotation, dtype)
- // once we fix the allocate tvm script printing.
+ ICHECK(IsPointerType(buffer_var->type_annotation, dtype))
+ << "Allocate: buffer_var expect to have the right pointer type annotation"
+ << " annotation=" << buffer_var->type_annotation << ", dtype=" << dtype;
Review comment:
This should probably be a `CHECK` too if people are writing tvmscript.
##########
File path: src/tir/ir/stmt.cc
##########
@@ -274,9 +274,10 @@ TVM_STATIC_IR_FUNCTOR(ReprPrinter, vtable)
// Allocate
Allocate::Allocate(Var buffer_var, DataType dtype, Array<PrimExpr> extents, PrimExpr condition,
Stmt body, Span span) {
- // TODO(tvm-team): Add invariant check to make sure
- // IsPointerPType(buffer_var->type_annotation, dtype)
- // once we fix the allocate tvm script printing.
+ ICHECK(IsPointerType(buffer_var->type_annotation, dtype))
+ << "Allocate: buffer_var expect to have the right pointer type annotation"
+ << " annotation=" << buffer_var->type_annotation << ", dtype=" << dtype;
Review comment:
This might be a better error message:
```suggestion
<< "The allocated data type (" << dtype << ") does not match the type annotation of the buffer " << buffer_var << " (" << buffer_var->type_annotation << "). The data type should be an element of the pointer type."
```
It does seem like we could infer the type from the buffer_var though...
----------------------------------------------------------------
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 #7216: [TIR][REFACTOR] Enforce allocate to use the correct var pointer hint.
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #7216:
URL: https://github.com/apache/tvm/pull/7216#discussion_r552316119
##########
File path: python/tvm/tir/buffer.py
##########
@@ -247,7 +247,10 @@ def decl_buffer(
shape_dtype = shape[0].dtype if hasattr(shape[0], "dtype") else "int32"
elem_offset = Var("%s_elem_offset" % name, shape_dtype)
if data is None:
- data = Var(name, PointerType(PrimType(dtype)), span)
+ # Bool is represented as uint1 in the IR, but stored as uint8
Review comment:
good catch
----------------------------------------------------------------
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 edited a comment on pull request #7216: [TIR][REFACTOR] Enforce allocate to use the correct var hint.
Posted by GitBox <gi...@apache.org>.
tqchen edited a comment on pull request #7216:
URL: https://github.com/apache/tvm/pull/7216#issuecomment-754921537
cc @spectrometerHBH @junrushao1994 @Hzfengsy @tkonolige @ZihengJiang
----------------------------------------------------------------
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 #7216: [TIR][REFACTOR] Enforce allocate to use the correct var pointer hint.
Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #7216:
URL: https://github.com/apache/tvm/pull/7216#issuecomment-755328940
Thanks @junrushao1994 @tkonolige comments are addressed
----------------------------------------------------------------
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] junrushao1994 merged pull request #7216: [TIR][REFACTOR] Enforce allocate to use the correct var pointer hint.
Posted by GitBox <gi...@apache.org>.
junrushao1994 merged pull request #7216:
URL: https://github.com/apache/tvm/pull/7216
----------------------------------------------------------------
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 #7216: [TIR][REFACTOR] Enforce allocate to use the correct var pointer hint.
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #7216:
URL: https://github.com/apache/tvm/pull/7216#discussion_r552251047
##########
File path: python/tvm/tir/buffer.py
##########
@@ -247,7 +247,9 @@ def decl_buffer(
shape_dtype = shape[0].dtype if hasattr(shape[0], "dtype") else "int32"
elem_offset = Var("%s_elem_offset" % name, shape_dtype)
if data is None:
- data = Var(name, PointerType(PrimType(dtype)), span)
+ # store bool as i8
+ storage_dtype = "int8" if dtype == "bool" else dtype
Review comment:
In the current convention, the bool are stored as int8, so the pointer type of the bool is `i8*`, while bool is represented as `i1` in the IR.
----------------------------------------------------------------
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 edited a comment on pull request #7216: [TIR][REFACTOR] Enforce allocate to use the correct var hint.
Posted by GitBox <gi...@apache.org>.
tqchen edited a comment on pull request #7216:
URL: https://github.com/apache/tvm/pull/7216#issuecomment-754921537
cc @spectrometerHBH @junrushao1994 @Hzfengsy @tkonolige @ZihengJiang @hypercubestart
----------------------------------------------------------------
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] hypercubestart commented on a change in pull request #7216: [TIR][REFACTOR] Enforce allocate to use the correct var pointer hint.
Posted by GitBox <gi...@apache.org>.
hypercubestart commented on a change in pull request #7216:
URL: https://github.com/apache/tvm/pull/7216#discussion_r552314233
##########
File path: python/tvm/tir/buffer.py
##########
@@ -247,7 +247,10 @@ def decl_buffer(
shape_dtype = shape[0].dtype if hasattr(shape[0], "dtype") else "int32"
elem_offset = Var("%s_elem_offset" % name, shape_dtype)
if data is None:
- data = Var(name, PointerType(PrimType(dtype)), span)
+ # Bool is represented as uint1 in the IR, but stored as uint8
Review comment:
is this supposed to be "stored as int8"?
----------------------------------------------------------------
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] junrushao1994 commented on a change in pull request #7216: [TIR][REFACTOR] Enforce allocate to use the correct var pointer hint.
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on a change in pull request #7216:
URL: https://github.com/apache/tvm/pull/7216#discussion_r552250872
##########
File path: python/tvm/tir/buffer.py
##########
@@ -247,7 +247,9 @@ def decl_buffer(
shape_dtype = shape[0].dtype if hasattr(shape[0], "dtype") else "int32"
elem_offset = Var("%s_elem_offset" % name, shape_dtype)
if data is None:
- data = Var(name, PointerType(PrimType(dtype)), span)
+ # store bool as i8
+ storage_dtype = "int8" if dtype == "bool" else dtype
Review comment:
I think bool is usually treated as "uint1" though...
----------------------------------------------------------------
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 #7216: [TIR][REFACTOR] Enforce allocate to use the correct var hint.
Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #7216:
URL: https://github.com/apache/tvm/pull/7216#issuecomment-754921537
cc @spectrometerHBH @junrushao1994 @Hzfengsy @tkonolige
----------------------------------------------------------------
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 #7216: [TIR][REFACTOR] Enforce allocate to use the correct var pointer hint.
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #7216:
URL: https://github.com/apache/tvm/pull/7216#discussion_r552838662
##########
File path: python/tvm/tir/buffer.py
##########
@@ -247,7 +247,9 @@ def decl_buffer(
shape_dtype = shape[0].dtype if hasattr(shape[0], "dtype") else "int32"
elem_offset = Var("%s_elem_offset" % name, shape_dtype)
if data is None:
- data = Var(name, PointerType(PrimType(dtype)), span)
+ # store bool as i8
+ storage_dtype = "int8" if dtype == "bool" else dtype
Review comment:
This would complicates the logic of the PrimType. Constructors are expected to be simple and "no surprise". Notably we still want to be able to represent `i1` types in the type system. It is just in common platforms i8* is used to store values of i1.
Considering the above reasons I don't think it is a good idea to have PrimType constructor to do the conversation.
----------------------------------------------------------------
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