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 2023/01/05 23:33:17 UTC
[GitHub] [tvm] masahi opened a new pull request, #13710: Dtype legalize fix let
masahi opened a new pull request, #13710:
URL: https://github.com/apache/tvm/pull/13710
Previously, even if the dtype of the rhs of `LetStmt` is casted to int64, the bound variable doesn't get updated.
--
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] masahi commented on a diff in pull request #13710: [TIR] Fix dtype mismatch error due to LetStmt
Posted by GitBox <gi...@apache.org>.
masahi commented on code in PR #13710:
URL: https://github.com/apache/tvm/pull/13710#discussion_r1063842517
##########
src/tir/ir/data_type_rewriter.cc:
##########
@@ -107,6 +107,26 @@ Stmt DataTypeLegalizer::VisitStmt_(const AttrStmtNode* op) {
return StmtExprMutator::VisitStmt_(op);
}
+Stmt DataTypeLegalizer::VisitStmt_(const LetStmtNode* op) {
+ PrimExpr value = this->VisitExpr(op->value);
+ Stmt body = this->VisitStmt(op->body);
+ if (value.same_as(op->value) && body.same_as(op->body)) {
+ return GetRef<Stmt>(op);
+ } else if (value.dtype() == op->var->dtype) {
+ auto n = CopyOnWrite(op);
+ n->value = std::move(value);
+ n->body = std::move(body);
+ return Stmt(n);
+ } else {
+ auto new_var = op->var.copy_with_dtype(value.dtype());
+ Map<Var, PrimExpr> vmap{{op->var, new_var}};
+ auto new_body = SubstituteWithDataTypeLegalization(
+ std::move(body), [&](const Var& var) { return vmap.Get(var); });
+ // We need to visit the body again to insert additional casts
Review Comment:
ok fixed, PTAL @vinx13
--
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] masahi commented on a diff in pull request #13710: [TIR] Fix dtype mismatch error due to LetStmt
Posted by GitBox <gi...@apache.org>.
masahi commented on code in PR #13710:
URL: https://github.com/apache/tvm/pull/13710#discussion_r1063847366
##########
src/tir/ir/data_type_rewriter.cc:
##########
@@ -107,6 +107,26 @@ Stmt DataTypeLegalizer::VisitStmt_(const AttrStmtNode* op) {
return StmtExprMutator::VisitStmt_(op);
}
+Stmt DataTypeLegalizer::VisitStmt_(const LetStmtNode* op) {
+ PrimExpr value = this->VisitExpr(op->value);
+ Stmt body = this->VisitStmt(op->body);
+ if (value.same_as(op->value) && body.same_as(op->body)) {
+ return GetRef<Stmt>(op);
+ } else if (value.dtype() == op->var->dtype) {
+ auto n = CopyOnWrite(op);
+ n->value = std::move(value);
+ n->body = std::move(body);
+ return Stmt(n);
+ } else {
+ auto new_var = op->var.copy_with_dtype(value.dtype());
+ Map<Var, PrimExpr> vmap{{op->var, new_var}};
+ auto new_body = SubstituteWithDataTypeLegalization(
+ std::move(body), [&](const Var& var) { return vmap.Get(var); });
+ // We need to visit the body again to insert additional casts
Review Comment:
I see, there is also `var_remap_` in the derived class, `IndexDataTypeNormalizer`. We may be able to unify
--
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] vinx13 commented on a diff in pull request #13710: [TIR] Fix dtype mismatch error due to LetStmt
Posted by GitBox <gi...@apache.org>.
vinx13 commented on code in PR #13710:
URL: https://github.com/apache/tvm/pull/13710#discussion_r1063841351
##########
src/tir/ir/data_type_rewriter.cc:
##########
@@ -107,6 +107,26 @@ Stmt DataTypeLegalizer::VisitStmt_(const AttrStmtNode* op) {
return StmtExprMutator::VisitStmt_(op);
}
+Stmt DataTypeLegalizer::VisitStmt_(const LetStmtNode* op) {
+ PrimExpr value = this->VisitExpr(op->value);
+ Stmt body = this->VisitStmt(op->body);
+ if (value.same_as(op->value) && body.same_as(op->body)) {
+ return GetRef<Stmt>(op);
+ } else if (value.dtype() == op->var->dtype) {
+ auto n = CopyOnWrite(op);
+ n->value = std::move(value);
+ n->body = std::move(body);
+ return Stmt(n);
+ } else {
+ auto new_var = op->var.copy_with_dtype(value.dtype());
+ Map<Var, PrimExpr> vmap{{op->var, new_var}};
+ auto new_body = SubstituteWithDataTypeLegalization(
+ std::move(body), [&](const Var& var) { return vmap.Get(var); });
+ // We need to visit the body again to insert additional casts
Review Comment:
`SubstituteWithDataTypeLegalization` is implemented with `DataTypeLegalizer`. So I'm suggesting instead of calling `SubstituteWithDataTypeLegalization`, we just update the vmap before recursive visiting. Something like:
```
PrimExpr VisitExpr_(const VarNode* op) {
if (auto it = vmap.find(op); it != vmap.end()) return (*it).second;
return GetRef<Var>(op);
}
Stmt DataTypeLegalizer::VisitStmt_(const LetStmtNode* op) {
PrimExpr new_value = this->VisitExpr(op->value);
if (op->var->dtype != new_value.dtype()) {
vmap[op->var] = new_var;
new_body = VisitStmt(op->body);
return LetStmt(new_var, new_value, new_body);
}
}
```
--
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] masahi commented on a diff in pull request #13710: [TIR] Fix dtype mismatch error due to LetStmt
Posted by GitBox <gi...@apache.org>.
masahi commented on code in PR #13710:
URL: https://github.com/apache/tvm/pull/13710#discussion_r1063815756
##########
src/tir/ir/data_type_rewriter.cc:
##########
@@ -107,6 +107,26 @@ Stmt DataTypeLegalizer::VisitStmt_(const AttrStmtNode* op) {
return StmtExprMutator::VisitStmt_(op);
}
+Stmt DataTypeLegalizer::VisitStmt_(const LetStmtNode* op) {
+ PrimExpr value = this->VisitExpr(op->value);
+ Stmt body = this->VisitStmt(op->body);
+ if (value.same_as(op->value) && body.same_as(op->body)) {
+ return GetRef<Stmt>(op);
+ } else if (value.dtype() == op->var->dtype) {
+ auto n = CopyOnWrite(op);
+ n->value = std::move(value);
+ n->body = std::move(body);
+ return Stmt(n);
+ } else {
+ auto new_var = op->var.copy_with_dtype(value.dtype());
+ Map<Var, PrimExpr> vmap{{op->var, new_var}};
+ auto new_body = SubstituteWithDataTypeLegalization(
+ std::move(body), [&](const Var& var) { return vmap.Get(var); });
+ // We need to visit the body again to insert additional casts
Review Comment:
We can pass `op->body` to `SubstituteWithDataTypeLegalization`, but we still need to visit `new_body` after `SubstituteWithDataTypeLegalization`. In that case, the output of the first visit,`this->VisitStmt(op->body)` at the top is only used for the first two `if / else` branches.
So this doesn't work, because the body of the Let has outdated variable dtype. Is this your understanding as well, or are you suggesting something else?
```
auto new_body = SubstituteWithDataTypeLegalization(
std::move(op->body), [&](const Var& var) { return vmap.Get(var); });
// We need to visit the body again to insert additional casts
return LetStmt(new_var, value, new_body, op->span);
```
--
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] masahi commented on a diff in pull request #13710: [TIR] Fix dtype mismatch error due to LetStmt
Posted by GitBox <gi...@apache.org>.
masahi commented on code in PR #13710:
URL: https://github.com/apache/tvm/pull/13710#discussion_r1063835555
##########
src/tir/ir/data_type_rewriter.cc:
##########
@@ -107,6 +107,26 @@ Stmt DataTypeLegalizer::VisitStmt_(const AttrStmtNode* op) {
return StmtExprMutator::VisitStmt_(op);
}
+Stmt DataTypeLegalizer::VisitStmt_(const LetStmtNode* op) {
+ PrimExpr value = this->VisitExpr(op->value);
+ Stmt body = this->VisitStmt(op->body);
+ if (value.same_as(op->value) && body.same_as(op->body)) {
+ return GetRef<Stmt>(op);
+ } else if (value.dtype() == op->var->dtype) {
+ auto n = CopyOnWrite(op);
+ n->value = std::move(value);
+ n->body = std::move(body);
+ return Stmt(n);
+ } else {
+ auto new_var = op->var.copy_with_dtype(value.dtype());
+ Map<Var, PrimExpr> vmap{{op->var, new_var}};
+ auto new_body = SubstituteWithDataTypeLegalization(
+ std::move(body), [&](const Var& var) { return vmap.Get(var); });
+ // We need to visit the body again to insert additional casts
Review Comment:
I'm experimenting with the following definition of `LetStmt` visitor, which looks better but it fails to pass the test for some reason (the cast before buffer store is not inserted). Looking.
```
Stmt DataTypeLegalizer::VisitStmt_(const LetStmtNode* op) {
PrimExpr value = this->VisitExpr(op->value);
auto body = op->body;
auto new_var = op->var.copy_with_dtype(value.dtype());
if (value.dtype() != op->var->dtype) {
Map<Var, PrimExpr> vmap{{op->var, new_var}};
body =
SubstituteWithDataTypeLegalization(op->body, [&](const Var& var) { return vmap.Get(var); });
}
Stmt new_body = this->VisitStmt(body);
if (value.same_as(op->value) && new_body.same_as(op->body)) {
return GetRef<Stmt>(op);
} else if (value.dtype() == op->var->dtype) {
auto n = CopyOnWrite(op);
n->value = std::move(value);
n->body = std::move(body);
return Stmt(n);
} else {
// We need to visit the body again to insert additional casts
return LetStmt(new_var, value, new_body, op->span);
}
}
```
--
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] masahi commented on a diff in pull request #13710: [TIR] Fix dtype mismatch error due to LetStmt
Posted by GitBox <gi...@apache.org>.
masahi commented on code in PR #13710:
URL: https://github.com/apache/tvm/pull/13710#discussion_r1063081290
##########
src/tir/ir/data_type_rewriter.cc:
##########
@@ -397,6 +417,9 @@ Stmt IndexDataTypeRewriter::VisitStmt_(const BufferStoreNode* op) {
Buffer new_buffer = GetRemappedBuffer(op->buffer);
auto value = this->VisitExpr(op->value);
+ if (new_buffer->dtype != value->dtype && value->dtype.lanes() == 1) {
+ value = cast(new_buffer->dtype, value);
Review Comment:
`new_buffer->dtype != value->dtype` is true if `new_buffer->dtype == float32` and `value->dtype == float32x16`, and the indices is a vector. `value->dtype.lanes() == 1` condition is added to prevent adding a cast in such cases.
For argmax, this cast can result in a redundant chain of casts of the form int32(select(cond, int64(tru), int64(false))), but hopefully LLVM or other backend can clean them up.
--
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] masahi commented on a diff in pull request #13710: [TIR] Fix dtype mismatch error due to LetStmt
Posted by GitBox <gi...@apache.org>.
masahi commented on code in PR #13710:
URL: https://github.com/apache/tvm/pull/13710#discussion_r1063844872
##########
src/tir/ir/data_type_rewriter.cc:
##########
@@ -107,6 +107,26 @@ Stmt DataTypeLegalizer::VisitStmt_(const AttrStmtNode* op) {
return StmtExprMutator::VisitStmt_(op);
}
+Stmt DataTypeLegalizer::VisitStmt_(const LetStmtNode* op) {
+ PrimExpr value = this->VisitExpr(op->value);
+ Stmt body = this->VisitStmt(op->body);
+ if (value.same_as(op->value) && body.same_as(op->body)) {
+ return GetRef<Stmt>(op);
+ } else if (value.dtype() == op->var->dtype) {
+ auto n = CopyOnWrite(op);
+ n->value = std::move(value);
+ n->body = std::move(body);
+ return Stmt(n);
+ } else {
+ auto new_var = op->var.copy_with_dtype(value.dtype());
+ Map<Var, PrimExpr> vmap{{op->var, new_var}};
+ auto new_body = SubstituteWithDataTypeLegalization(
+ std::move(body), [&](const Var& var) { return vmap.Get(var); });
+ // We need to visit the body again to insert additional casts
Review Comment:
@vinx13 Not sure where that `vmap` is coming from?
--
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] vinx13 commented on a diff in pull request #13710: [TIR] Fix dtype mismatch error due to LetStmt
Posted by GitBox <gi...@apache.org>.
vinx13 commented on code in PR #13710:
URL: https://github.com/apache/tvm/pull/13710#discussion_r1063673395
##########
src/tir/ir/data_type_rewriter.cc:
##########
@@ -107,6 +107,26 @@ Stmt DataTypeLegalizer::VisitStmt_(const AttrStmtNode* op) {
return StmtExprMutator::VisitStmt_(op);
}
+Stmt DataTypeLegalizer::VisitStmt_(const LetStmtNode* op) {
+ PrimExpr value = this->VisitExpr(op->value);
+ Stmt body = this->VisitStmt(op->body);
+ if (value.same_as(op->value) && body.same_as(op->body)) {
+ return GetRef<Stmt>(op);
+ } else if (value.dtype() == op->var->dtype) {
+ auto n = CopyOnWrite(op);
+ n->value = std::move(value);
+ n->body = std::move(body);
+ return Stmt(n);
+ } else {
+ auto new_var = op->var.copy_with_dtype(value.dtype());
+ Map<Var, PrimExpr> vmap{{op->var, new_var}};
+ auto new_body = SubstituteWithDataTypeLegalization(
+ std::move(body), [&](const Var& var) { return vmap.Get(var); });
+ // We need to visit the body again to insert additional casts
Review Comment:
it's possible to do this without revisiting. We can only visit value at the beginning, and update the vmap before visiting body
--
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] masahi commented on a diff in pull request #13710: [TIR] Fix dtype mismatch error due to LetStmt
Posted by GitBox <gi...@apache.org>.
masahi commented on code in PR #13710:
URL: https://github.com/apache/tvm/pull/13710#discussion_r1063842929
##########
src/tir/ir/data_type_rewriter.cc:
##########
@@ -107,6 +107,26 @@ Stmt DataTypeLegalizer::VisitStmt_(const AttrStmtNode* op) {
return StmtExprMutator::VisitStmt_(op);
}
+Stmt DataTypeLegalizer::VisitStmt_(const LetStmtNode* op) {
+ PrimExpr value = this->VisitExpr(op->value);
+ Stmt body = this->VisitStmt(op->body);
+ if (value.same_as(op->value) && body.same_as(op->body)) {
+ return GetRef<Stmt>(op);
+ } else if (value.dtype() == op->var->dtype) {
+ auto n = CopyOnWrite(op);
+ n->value = std::move(value);
+ n->body = std::move(body);
+ return Stmt(n);
+ } else {
+ auto new_var = op->var.copy_with_dtype(value.dtype());
+ Map<Var, PrimExpr> vmap{{op->var, new_var}};
+ auto new_body = SubstituteWithDataTypeLegalization(
+ std::move(body), [&](const Var& var) { return vmap.Get(var); });
+ // We need to visit the body again to insert additional casts
Review Comment:
sorry just saw your comment, looking now.
--
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] masahi commented on a diff in pull request #13710: [TIR] Fix dtype mismatch error due to LetStmt
Posted by GitBox <gi...@apache.org>.
masahi commented on code in PR #13710:
URL: https://github.com/apache/tvm/pull/13710#discussion_r1063891653
##########
src/tir/ir/data_type_rewriter.cc:
##########
@@ -107,6 +107,26 @@ Stmt DataTypeLegalizer::VisitStmt_(const AttrStmtNode* op) {
return StmtExprMutator::VisitStmt_(op);
}
+Stmt DataTypeLegalizer::VisitStmt_(const LetStmtNode* op) {
+ PrimExpr value = this->VisitExpr(op->value);
+ Stmt body = this->VisitStmt(op->body);
+ if (value.same_as(op->value) && body.same_as(op->body)) {
+ return GetRef<Stmt>(op);
+ } else if (value.dtype() == op->var->dtype) {
+ auto n = CopyOnWrite(op);
+ n->value = std::move(value);
+ n->body = std::move(body);
+ return Stmt(n);
+ } else {
+ auto new_var = op->var.copy_with_dtype(value.dtype());
+ Map<Var, PrimExpr> vmap{{op->var, new_var}};
+ auto new_body = SubstituteWithDataTypeLegalization(
+ std::move(body), [&](const Var& var) { return vmap.Get(var); });
+ // We need to visit the body again to insert additional casts
Review Comment:
done
--
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] tvm-bot commented on pull request #13710: Dtype legalize fix let
Posted by GitBox <gi...@apache.org>.
tvm-bot commented on PR #13710:
URL: https://github.com/apache/tvm/pull/13710#issuecomment-1372928522
<!---bot-comment-->
Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from [Reviewers](https://github.com/apache/incubator-tvm/blob/master/CONTRIBUTORS.md#reviewers) by @-ing them in a comment.
<!--bot-comment-ccs-start-->
* No users to auto-tag found, no teams are specified in PR title <sub>See [#10317](https://github.com/apache/tvm/issues/10317) for details</sub><!--bot-comment-ccs-end-->
<sub>Generated by [tvm-bot](https://github.com/apache/tvm/blob/main/ci/README.md#github-actions)</sub>
--
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] masahi merged pull request #13710: [TIR] Fix dtype mismatch error due to LetStmt
Posted by GitBox <gi...@apache.org>.
masahi merged PR #13710:
URL: https://github.com/apache/tvm/pull/13710
--
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] masahi commented on a diff in pull request #13710: [TIR] Fix dtype mismatch error due to LetStmt
Posted by GitBox <gi...@apache.org>.
masahi commented on code in PR #13710:
URL: https://github.com/apache/tvm/pull/13710#discussion_r1063081290
##########
src/tir/ir/data_type_rewriter.cc:
##########
@@ -397,6 +417,9 @@ Stmt IndexDataTypeRewriter::VisitStmt_(const BufferStoreNode* op) {
Buffer new_buffer = GetRemappedBuffer(op->buffer);
auto value = this->VisitExpr(op->value);
+ if (new_buffer->dtype != value->dtype && value->dtype.lanes() == 1) {
+ value = cast(new_buffer->dtype, value);
Review Comment:
`new_buffer->dtype != value->dtype` is true if `new_buffer->dtype == float32` and `value->dtype == float32x16`, and the indices is a vector. `value->dtype.lanes() == 1` condition is added to prevent adding a cast in such cases.
For argmax, this cast can result in a redundant chain of casts of the form `int32(select(cond, int64(tru), int64(false)))`, but hopefully LLVM or other backend can clean them up.
--
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] vinx13 commented on a diff in pull request #13710: [TIR] Fix dtype mismatch error due to LetStmt
Posted by GitBox <gi...@apache.org>.
vinx13 commented on code in PR #13710:
URL: https://github.com/apache/tvm/pull/13710#discussion_r1063845468
##########
src/tir/ir/data_type_rewriter.cc:
##########
@@ -107,6 +107,26 @@ Stmt DataTypeLegalizer::VisitStmt_(const AttrStmtNode* op) {
return StmtExprMutator::VisitStmt_(op);
}
+Stmt DataTypeLegalizer::VisitStmt_(const LetStmtNode* op) {
+ PrimExpr value = this->VisitExpr(op->value);
+ Stmt body = this->VisitStmt(op->body);
+ if (value.same_as(op->value) && body.same_as(op->body)) {
+ return GetRef<Stmt>(op);
+ } else if (value.dtype() == op->var->dtype) {
+ auto n = CopyOnWrite(op);
+ n->value = std::move(value);
+ n->body = std::move(body);
+ return Stmt(n);
+ } else {
+ auto new_var = op->var.copy_with_dtype(value.dtype());
+ Map<Var, PrimExpr> vmap{{op->var, new_var}};
+ auto new_body = SubstituteWithDataTypeLegalization(
+ std::move(body), [&](const Var& var) { return vmap.Get(var); });
+ // We need to visit the body again to insert additional casts
Review Comment:
oh I'm suggesting adding a new `vmap_` field
--
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