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 2022/04/26 14:44:29 UTC
[GitHub] [tvm] Lunderberg opened a new pull request, #11130: [Analysis] Readability/Deduplication in Analyzer CanProve/Simplify
Lunderberg opened a new pull request, #11130:
URL: https://github.com/apache/tvm/pull/11130
- Remove duplication between `Analyzer::CanProve` and `Analyzer::Simplify`.
- Simplify loop iteration in `Analyzer::Simplify`. The `++i` previously in the middle of the loop was easy to miss.
--
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] Lunderberg commented on a diff in pull request #11130: [Analysis] Readability/Deduplication in Analyzer CanProve/Simplify
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on code in PR #11130:
URL: https://github.com/apache/tvm/pull/11130#discussion_r858860779
##########
src/arith/analyzer.cc:
##########
@@ -108,29 +108,25 @@ bool Analyzer::CanProveEqual(const PrimExpr& lhs, const PrimExpr& rhs) {
}
bool Analyzer::CanProve(const PrimExpr& expr) {
- if (const auto* ptr = expr.as<IntImmNode>()) {
- return ptr->value != 0;
- }
- auto res = this->rewrite_simplify(expr);
- if (const auto* ptr = res.as<IntImmNode>()) {
- return ptr->value != 0;
- }
- res = this->canonical_simplify(expr);
- if (const auto* ptr = res.as<IntImmNode>()) {
- return ptr->value != 0;
- }
- return false;
+ PrimExpr simplified = Simplify(expr);
+ const int64_t* as_int = tir::as_const_int(simplified);
+ return as_int && *as_int;
}
PrimExpr Analyzer::Simplify(const PrimExpr& expr, int steps) {
- if (tir::is_const_int(expr)) return expr;
PrimExpr res = expr;
- for (int i = 0; i < steps; ++i) {
- res = this->rewrite_simplify(res);
- if (tir::is_const_int(res) || ++i == steps) return res;
- res = this->canonical_simplify(res);
- if (tir::is_const_int(res)) return res;
+
+ for (int i = 0; i < steps; i++) {
+ if (tir::is_const_int(res)) {
Review Comment:
Sounds good and changed. The break and the early return both result in the same optimized assembly in g++, so I initially went with the break because I found it easier to read.
--
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] tqchen commented on a diff in pull request #11130: [Analysis] Readability/Deduplication in Analyzer CanProve/Simplify
Posted by GitBox <gi...@apache.org>.
tqchen commented on code in PR #11130:
URL: https://github.com/apache/tvm/pull/11130#discussion_r861760487
##########
src/arith/analyzer.cc:
##########
@@ -108,29 +108,25 @@ bool Analyzer::CanProveEqual(const PrimExpr& lhs, const PrimExpr& rhs) {
}
bool Analyzer::CanProve(const PrimExpr& expr) {
- if (const auto* ptr = expr.as<IntImmNode>()) {
- return ptr->value != 0;
Review Comment:
hmm i still see the two lines deleted, perhaps a miss that forgets to checkin?
--
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] tqchen commented on a diff in pull request #11130: [Analysis] Readability/Deduplication in Analyzer CanProve/Simplify
Posted by GitBox <gi...@apache.org>.
tqchen commented on code in PR #11130:
URL: https://github.com/apache/tvm/pull/11130#discussion_r858858578
##########
src/arith/analyzer.cc:
##########
@@ -108,29 +108,25 @@ bool Analyzer::CanProveEqual(const PrimExpr& lhs, const PrimExpr& rhs) {
}
bool Analyzer::CanProve(const PrimExpr& expr) {
- if (const auto* ptr = expr.as<IntImmNode>()) {
- return ptr->value != 0;
Review Comment:
I agree with what you said(simplify also starts to be low cost), so I could be over simplifying here :) In the meantime being explicit without relying on subsequent behavior (which would be one cost of call) can be helpful a bit.
--
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] tqchen merged pull request #11130: [Analysis] Readability/Deduplication in Analyzer CanProve/Simplify
Posted by GitBox <gi...@apache.org>.
tqchen merged PR #11130:
URL: https://github.com/apache/tvm/pull/11130
--
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] Lunderberg commented on a diff in pull request #11130: [Analysis] Readability/Deduplication in Analyzer CanProve/Simplify
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on code in PR #11130:
URL: https://github.com/apache/tvm/pull/11130#discussion_r858857024
##########
src/arith/analyzer.cc:
##########
@@ -108,29 +108,25 @@ bool Analyzer::CanProveEqual(const PrimExpr& lhs, const PrimExpr& rhs) {
}
bool Analyzer::CanProve(const PrimExpr& expr) {
- if (const auto* ptr = expr.as<IntImmNode>()) {
- return ptr->value != 0;
- }
- auto res = this->rewrite_simplify(expr);
- if (const auto* ptr = res.as<IntImmNode>()) {
- return ptr->value != 0;
- }
- res = this->canonical_simplify(expr);
- if (const auto* ptr = res.as<IntImmNode>()) {
- return ptr->value != 0;
- }
- return false;
+ PrimExpr simplified = Simplify(expr);
+ const int64_t* as_int = tir::as_const_int(simplified);
+ return as_int && *as_int;
}
PrimExpr Analyzer::Simplify(const PrimExpr& expr, int steps) {
- if (tir::is_const_int(expr)) return expr;
PrimExpr res = expr;
- for (int i = 0; i < steps; ++i) {
- res = this->rewrite_simplify(res);
- if (tir::is_const_int(res) || ++i == steps) return res;
- res = this->canonical_simplify(res);
- if (tir::is_const_int(res)) return res;
+
+ for (int i = 0; i < steps; i++) {
Review Comment:
Sounds good and changed. This results in the same optimized assembly, so I haven't been picky on it for primitive types.
--
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] Lunderberg commented on a diff in pull request #11130: [Analysis] Readability/Deduplication in Analyzer CanProve/Simplify
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on code in PR #11130:
URL: https://github.com/apache/tvm/pull/11130#discussion_r861803347
##########
src/arith/analyzer.cc:
##########
@@ -108,29 +108,25 @@ bool Analyzer::CanProveEqual(const PrimExpr& lhs, const PrimExpr& rhs) {
}
bool Analyzer::CanProve(const PrimExpr& expr) {
- if (const auto* ptr = expr.as<IntImmNode>()) {
- return ptr->value != 0;
Review Comment:
Hmm, the "Files changed" tab on my side shows a single line added with the comment. If I look at it for each commit in the PR, I see the two lines deleted in the commit 20c8f08, then added back in in d1181a3 along with the comment.
--
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] tqchen commented on a diff in pull request #11130: [Analysis] Readability/Deduplication in Analyzer CanProve/Simplify
Posted by GitBox <gi...@apache.org>.
tqchen commented on code in PR #11130:
URL: https://github.com/apache/tvm/pull/11130#discussion_r861824855
##########
src/arith/analyzer.cc:
##########
@@ -108,29 +108,25 @@ bool Analyzer::CanProveEqual(const PrimExpr& lhs, const PrimExpr& rhs) {
}
bool Analyzer::CanProve(const PrimExpr& expr) {
- if (const auto* ptr = expr.as<IntImmNode>()) {
- return ptr->value != 0;
Review Comment:
Looking again and it seems to be oK, my bad. This is approved
--
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] tqchen commented on a diff in pull request #11130: [Analysis] Readability/Deduplication in Analyzer CanProve/Simplify
Posted by GitBox <gi...@apache.org>.
tqchen commented on code in PR #11130:
URL: https://github.com/apache/tvm/pull/11130#discussion_r858811721
##########
src/arith/analyzer.cc:
##########
@@ -108,29 +108,25 @@ bool Analyzer::CanProveEqual(const PrimExpr& lhs, const PrimExpr& rhs) {
}
bool Analyzer::CanProve(const PrimExpr& expr) {
- if (const auto* ptr = expr.as<IntImmNode>()) {
- return ptr->value != 0;
- }
- auto res = this->rewrite_simplify(expr);
- if (const auto* ptr = res.as<IntImmNode>()) {
- return ptr->value != 0;
- }
- res = this->canonical_simplify(expr);
- if (const auto* ptr = res.as<IntImmNode>()) {
- return ptr->value != 0;
- }
- return false;
+ PrimExpr simplified = Simplify(expr);
+ const int64_t* as_int = tir::as_const_int(simplified);
+ return as_int && *as_int;
}
PrimExpr Analyzer::Simplify(const PrimExpr& expr, int steps) {
- if (tir::is_const_int(expr)) return expr;
PrimExpr res = expr;
- for (int i = 0; i < steps; ++i) {
- res = this->rewrite_simplify(res);
- if (tir::is_const_int(res) || ++i == steps) return res;
- res = this->canonical_simplify(res);
- if (tir::is_const_int(res)) return res;
+
+ for (int i = 0; i < steps; i++) {
Review Comment:
nit: prefer ++i over i++
--
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] Lunderberg commented on pull request #11130: [Analysis] Readability/Deduplication in Analyzer CanProve/Simplify
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on PR #11130:
URL: https://github.com/apache/tvm/pull/11130#issuecomment-1112524959
Rebase onto main to restart CI. Previous build failures seem to be unrelated issue on MacOS.
--
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] tqchen commented on pull request #11130: [Analysis] Readability/Deduplication in Analyzer CanProve/Simplify
Posted by GitBox <gi...@apache.org>.
tqchen commented on PR #11130:
URL: https://github.com/apache/tvm/pull/11130#issuecomment-1113334861
Thank you @Lunderberg !
--
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] tqchen commented on a diff in pull request #11130: [Analysis] Readability/Deduplication in Analyzer CanProve/Simplify
Posted by GitBox <gi...@apache.org>.
tqchen commented on code in PR #11130:
URL: https://github.com/apache/tvm/pull/11130#discussion_r858811057
##########
src/arith/analyzer.cc:
##########
@@ -108,29 +108,25 @@ bool Analyzer::CanProveEqual(const PrimExpr& lhs, const PrimExpr& rhs) {
}
bool Analyzer::CanProve(const PrimExpr& expr) {
- if (const auto* ptr = expr.as<IntImmNode>()) {
- return ptr->value != 0;
- }
- auto res = this->rewrite_simplify(expr);
- if (const auto* ptr = res.as<IntImmNode>()) {
- return ptr->value != 0;
- }
- res = this->canonical_simplify(expr);
- if (const auto* ptr = res.as<IntImmNode>()) {
- return ptr->value != 0;
- }
- return false;
+ PrimExpr simplified = Simplify(expr);
+ const int64_t* as_int = tir::as_const_int(simplified);
+ return as_int && *as_int;
}
PrimExpr Analyzer::Simplify(const PrimExpr& expr, int steps) {
- if (tir::is_const_int(expr)) return expr;
PrimExpr res = expr;
- for (int i = 0; i < steps; ++i) {
- res = this->rewrite_simplify(res);
- if (tir::is_const_int(res) || ++i == steps) return res;
- res = this->canonical_simplify(res);
- if (tir::is_const_int(res)) return res;
+
+ for (int i = 0; i < steps; i++) {
+ if (tir::is_const_int(res)) {
Review Comment:
we can do return res here.
##########
src/arith/analyzer.cc:
##########
@@ -108,29 +108,25 @@ bool Analyzer::CanProveEqual(const PrimExpr& lhs, const PrimExpr& rhs) {
}
bool Analyzer::CanProve(const PrimExpr& expr) {
- if (const auto* ptr = expr.as<IntImmNode>()) {
- return ptr->value != 0;
Review Comment:
These are quick path that we intended leave as it is to avoid trigerring of recursive function calling cost. Given CanProve can be called frequently, it would be useful to keep it as it is in this case.
We can document the rationle using comments
--
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] Lunderberg commented on a diff in pull request #11130: [Analysis] Readability/Deduplication in Analyzer CanProve/Simplify
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on code in PR #11130:
URL: https://github.com/apache/tvm/pull/11130#discussion_r858851943
##########
src/arith/analyzer.cc:
##########
@@ -108,29 +108,25 @@ bool Analyzer::CanProveEqual(const PrimExpr& lhs, const PrimExpr& rhs) {
}
bool Analyzer::CanProve(const PrimExpr& expr) {
- if (const auto* ptr = expr.as<IntImmNode>()) {
- return ptr->value != 0;
Review Comment:
Sounds good, and added back in. I hadn't done any performance tests, but had been expecting that the call to Simplify would be low cost as it starts by checking whether the value is a constant.
--
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