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 2020/07/29 07:17:18 UTC
[GitHub] [incubator-tvm] ANSHUMAN87 commented on a change in pull request #6147: [TIR][Logger]Buffer logger assert removed
ANSHUMAN87 commented on a change in pull request #6147:
URL: https://github.com/apache/incubator-tvm/pull/6147#discussion_r461322467
##########
File path: src/printer/tir_text_printer.cc
##########
@@ -203,8 +185,36 @@ Doc TIRTextPrinter::PrintRange(const RangeNode* op) {
Doc TIRTextPrinter::PrintBuffer(const BufferNode* op) {
const Buffer& buffer = GetRef<Buffer>(op);
- CHECK_GT(memo_buf_.count(buffer), 0);
- return meta_->InMeta(buffer) ? meta_->GetMetaNode(buffer) : memo_buf_[buffer];
+
+ if (meta_->InMeta(buffer)) {
+ return meta_->GetMetaNode(buffer);
+ } else if (memo_buf_.count(buffer)) {
+ return memo_buf_[buffer];
+ } else {
+ memo_buf_[buffer] = AllocBuf(buffer);
+ return BufferNode2Doc(op, memo_buf_[buffer]);
Review comment:
This is the case when a Buffer is encountered for first time.
I believe, the first case is always the declaration, so it needs to have all the info related to the buffer.
Later on it will just use the unique_name always.
Below is a snapshot post my change:
```
attr [T_full_like: Buffer(T_full_like_1: handle, float32, [2, 3, 4], [])] "realize_scope" = "";
realize(T_full_like, [0:2, 0:3, 0:4], True {
for (ax0.ax1.fused: int32, 0, 6) "parallel" {
for (ax2.inner: int32, 0, 4) "vectorized" {
T_full_like[floordiv(ax0.ax1.fused, 3), floormod(ax0.ax1.fused, 3), ax2.inner] = 0f32
}
}
})
```
In above sample `T_full_like` is the buffer.
Please let me know, if my understanding is wrong here. TIA!
##########
File path: src/printer/tir_text_printer.cc
##########
@@ -203,8 +185,36 @@ Doc TIRTextPrinter::PrintRange(const RangeNode* op) {
Doc TIRTextPrinter::PrintBuffer(const BufferNode* op) {
const Buffer& buffer = GetRef<Buffer>(op);
- CHECK_GT(memo_buf_.count(buffer), 0);
- return meta_->InMeta(buffer) ? meta_->GetMetaNode(buffer) : memo_buf_[buffer];
+
+ if (meta_->InMeta(buffer)) {
+ return meta_->GetMetaNode(buffer);
+ } else if (memo_buf_.count(buffer)) {
+ return memo_buf_[buffer];
+ } else {
+ memo_buf_[buffer] = AllocBuf(buffer);
+ return BufferNode2Doc(op, memo_buf_[buffer]);
Review comment:
Thanks! This also looks good to me. But i am not sure how to achieve it.
Can you please share some pointers, how to do that before encountering the Buffer ?
Like in the example, the sequence of nodes are as below:
`AttrStmtNode(First encounter of Buffer) -> BufferRealizeNode -> ForNode ....`
Would you please help me, how to realize your point in this case ?
##########
File path: src/printer/tir_text_printer.cc
##########
@@ -203,8 +185,36 @@ Doc TIRTextPrinter::PrintRange(const RangeNode* op) {
Doc TIRTextPrinter::PrintBuffer(const BufferNode* op) {
const Buffer& buffer = GetRef<Buffer>(op);
- CHECK_GT(memo_buf_.count(buffer), 0);
- return meta_->InMeta(buffer) ? meta_->GetMetaNode(buffer) : memo_buf_[buffer];
+
+ if (meta_->InMeta(buffer)) {
+ return meta_->GetMetaNode(buffer);
+ } else if (memo_buf_.count(buffer)) {
+ return memo_buf_[buffer];
+ } else {
+ memo_buf_[buffer] = AllocBuf(buffer);
+ return BufferNode2Doc(op, memo_buf_[buffer]);
Review comment:
Thanks! I can see your point clearly now.
But there are few points i like to be clear on, before adopting it.
1: This case is not `PrimFunc`. In this case it is `AttrStmtNode`. So there will be scope specific buffers and blocks. Given that if we club all buffer info at one place (like header as you suggested), it might be hard to correlate for reader.
For example below:
```
attr [0] "extern_scope" = 0;
attr [new_size: Buffer(new_size_1: Pointer(int64), int64, [1], [])] "realize_scope" = "global";
realize(new_size, [0:1], True {
attr [old_size: Buffer(old_size_1: Pointer(int64), int64, [1], [])] "realize_scope" = "global";
realize(old_size, [0:1], True {
attr [skip: Buffer(skip_1: Pointer(int32), int32, [1], [])] "realize_scope" = "global";
realize(skip, [0:1], True {
attr [infer_idx: Buffer(infer_idx_1: Pointer(int32), int32, [1], [])] "realize_scope" = "global";
realize(infer_idx, [0:1], True {
attr [dst_idx: Buffer(dst_idx_1: Pointer(int32), int32, [1], [])] "realize_scope" = "global";
realize(dst_idx, [0:1], True {
attr [src_idx: Buffer(src_idx_1: Pointer(int32), int32, [1], [])] "realize_scope" = "global";
realize(src_idx, [0:1], True {
attr [data_shape: Buffer(data_shape_1: Pointer(int64), int64, [3], [])] "realize_scope" = "global";
realize(data_shape, [0:3], True {
```
2: As this cant be handled in a generic way, there will be always a case where we need to handle it. For now i can handle it for `AttrStmtNode`, later on there can be more cases similar. So is that ok?
Please let me know your thoughts on above points. If required we can pull others too :)
##########
File path: src/printer/tir_text_printer.cc
##########
@@ -203,8 +185,36 @@ Doc TIRTextPrinter::PrintRange(const RangeNode* op) {
Doc TIRTextPrinter::PrintBuffer(const BufferNode* op) {
const Buffer& buffer = GetRef<Buffer>(op);
- CHECK_GT(memo_buf_.count(buffer), 0);
- return meta_->InMeta(buffer) ? meta_->GetMetaNode(buffer) : memo_buf_[buffer];
+
+ if (meta_->InMeta(buffer)) {
+ return meta_->GetMetaNode(buffer);
+ } else if (memo_buf_.count(buffer)) {
+ return memo_buf_[buffer];
+ } else {
+ memo_buf_[buffer] = AllocBuf(buffer);
+ return BufferNode2Doc(op, memo_buf_[buffer]);
Review comment:
Thanks a lot @spectrometerHBH for your valuable opinion. I am in total agreement with you. Its just that we cover all the aspects of it :)
Let us have some more opinion on this point.
@tqchen , @junrushao1994, @FrozenGene, @MarisaKirisame , @zhiics : Would you please share your thoughts on above points.
##########
File path: src/printer/tir_text_printer.cc
##########
@@ -203,8 +185,36 @@ Doc TIRTextPrinter::PrintRange(const RangeNode* op) {
Doc TIRTextPrinter::PrintBuffer(const BufferNode* op) {
const Buffer& buffer = GetRef<Buffer>(op);
- CHECK_GT(memo_buf_.count(buffer), 0);
- return meta_->InMeta(buffer) ? meta_->GetMetaNode(buffer) : memo_buf_[buffer];
+
+ if (meta_->InMeta(buffer)) {
+ return meta_->GetMetaNode(buffer);
+ } else if (memo_buf_.count(buffer)) {
+ return memo_buf_[buffer];
+ } else {
+ memo_buf_[buffer] = AllocBuf(buffer);
+ return BufferNode2Doc(op, memo_buf_[buffer]);
Review comment:
Thanks @tqchen for sharing your opinion on this.
I believe if i am not mistaken, i will close this comment section as the current changes takes care of logging Buffer details only once when it is encountered for first time(assumed to be the declaration point) and use the unique_id for later on references.
----------------------------------------------------------------
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