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/03/18 23:36:35 UTC
[GitHub] [tvm] tkonolige opened a new pull request #10686: [AUTO_SCHEDULER] Supported CSE (variable definitions) in feature extraction
tkonolige opened a new pull request #10686:
URL: https://github.com/apache/tvm/pull/10686
Add supported for LetStmts in feature extraction. A stack of variable definitions is maintained and added to the arithmetic analyzer at the appropriate points. The buffer access analysis now creates a new arithmetic analysis context per set of loops to avoid redefining variables which is unsafe in the presence of let statements.
@tqchen @junrushao1994 @merrymercy
--
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] tkonolige commented on a change in pull request #10686: [AUTO_SCHEDULER] Supported CSE (variable definitions) in feature extraction
Posted by GitBox <gi...@apache.org>.
tkonolige commented on a change in pull request #10686:
URL: https://github.com/apache/tvm/pull/10686#discussion_r831341749
##########
File path: src/auto_scheduler/feature.cc
##########
@@ -233,14 +233,9 @@ AnnotationPosType GetAnnotationPosEncoding(const Var& var, const Array<PrimExpr>
}
}
-// Return the extent of a for loop
-int64_t GetLoopExtent(const ForNode* node) {
- auto pint = node->extent.as<IntImmNode>();
- if (pint != nullptr) {
- return pint->value;
- } else {
- return 1;
- }
+// Return the maximum extent of a for loop
+int64_t GetLoopExtent(const ForNode* node, const Analyzer& ana) {
Review comment:
I've added a check that return 1 if const_int_bound returns inf (it way of saying it cannot analyze).
--
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 change in pull request #10686: [AUTO_SCHEDULER] Supported CSE (variable definitions) in feature extraction
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #10686:
URL: https://github.com/apache/tvm/pull/10686#discussion_r831382689
##########
File path: tests/python/unittest/test_auto_scheduler_feature.py
##########
@@ -220,12 +220,15 @@ def tir_matmul(
def test_primfunc():
- features = auto_scheduler.feature.named_features_from_primfunc(tir_matmul)
Review comment:
Great, seems our interpretations aligns with each other. I agree that having a test that thread through lowered is useful if we bring in future usecases that applies feature extractor on lowered code.
To summarize the recommendation:
- Keep the original `test_primfunc`(perhaps rename to `test_prim_func_before_lowering` to clairfy the intend)
- This was the original intended behavior of the extractor and auto-scheduler only relied on this behavior.
- Add a new test `test_primfunc_lowered_with_let`
- I understand that there is an exploration effort to try out this and possibly followup features that depends on this. Although auto-scheduler do not yet rely on this behavior, so it would be good to keep it separate.
- Like you said it is more like an integration tests(but i agree in this case it is OK to keep it here)
- (Optional): add a unit-test testcase with explicitly constructed tvmscript that covers the `lowered_let`. The main rationale is that the intend of the unit-test case would be clear and it becomes easier to debug in facing regression, but I feel in this case it can be optional.
--
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] junrushao1994 merged pull request #10686: [AUTO_SCHEDULER] Supported CSE (variable definitions) in feature extraction
Posted by GitBox <gi...@apache.org>.
junrushao1994 merged pull request #10686:
URL: https://github.com/apache/tvm/pull/10686
--
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] tkonolige commented on a change in pull request #10686: [AUTO_SCHEDULER] Supported CSE (variable definitions) in feature extraction
Posted by GitBox <gi...@apache.org>.
tkonolige commented on a change in pull request #10686:
URL: https://github.com/apache/tvm/pull/10686#discussion_r831318078
##########
File path: src/auto_scheduler/feature.cc
##########
@@ -724,6 +731,15 @@ class PerStoreFeatureExtractor : public StmtExprVisitor {
ExtractAllocationFeature(node);
}
+ void VisitStmt_(const LetStmtNode* node) final {
+ // TODO(tkonolige): add arithmetic counts from this statement to counts of inner stores.
+ ana_.Bind(node->var, node->value);
+ ICHECK(variable_definition_stack_.size() > 0)
+ << "Variable definition out size of a for loop is not handled by feature extraction";
+ variable_definition_stack_.back().push_back(std::make_tuple(node->var, node->value));
+ StmtExprVisitor::VisitStmt_(node);
Review comment:
No, the variable_definition_stack_ has entries on a per loop basis not a per let stmt basis.
--
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] tkonolige commented on pull request #10686: [AUTO_SCHEDULER] Supported CSE (variable definitions) in feature extraction
Posted by GitBox <gi...@apache.org>.
tkonolige commented on pull request #10686:
URL: https://github.com/apache/tvm/pull/10686#issuecomment-1080892142
@junrushao1994 Could you review again.
--
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] tkonolige commented on a change in pull request #10686: [AUTO_SCHEDULER] Supported CSE (variable definitions) in feature extraction
Posted by GitBox <gi...@apache.org>.
tkonolige commented on a change in pull request #10686:
URL: https://github.com/apache/tvm/pull/10686#discussion_r831389321
##########
File path: tests/python/unittest/test_auto_scheduler_feature.py
##########
@@ -220,12 +220,15 @@ def tir_matmul(
def test_primfunc():
- features = auto_scheduler.feature.named_features_from_primfunc(tir_matmul)
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] tqchen commented on a change in pull request #10686: [AUTO_SCHEDULER] Supported CSE (variable definitions) in feature extraction
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #10686:
URL: https://github.com/apache/tvm/pull/10686#discussion_r830507591
##########
File path: src/auto_scheduler/feature.cc
##########
@@ -233,14 +233,9 @@ AnnotationPosType GetAnnotationPosEncoding(const Var& var, const Array<PrimExpr>
}
}
-// Return the extent of a for loop
-int64_t GetLoopExtent(const ForNode* node) {
- auto pint = node->extent.as<IntImmNode>();
- if (pint != nullptr) {
- return pint->value;
- } else {
- return 1;
- }
+// Return the maximum extent of a for loop
+int64_t GetLoopExtent(const ForNode* node, const Analyzer& ana) {
Review comment:
NOTE: this is a change of behavior that might worth checking, const_int bound may return INT_MAX for cases that it cannot analyze
--
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] tkonolige commented on a change in pull request #10686: [AUTO_SCHEDULER] Supported CSE (variable definitions) in feature extraction
Posted by GitBox <gi...@apache.org>.
tkonolige commented on a change in pull request #10686:
URL: https://github.com/apache/tvm/pull/10686#discussion_r831313440
##########
File path: tests/python/unittest/test_auto_scheduler_feature.py
##########
@@ -220,12 +220,15 @@ def tir_matmul(
def test_primfunc():
- features = auto_scheduler.feature.named_features_from_primfunc(tir_matmul)
Review comment:
I wrote the original test and its intended usage was for analysis after lowering.
I specifically did not want to construct a test from the explicitly lowered tvmscript so future large changes like CSE would cause the test to fail.
--
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 change in pull request #10686: [AUTO_SCHEDULER] Supported CSE (variable definitions) in feature extraction
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #10686:
URL: https://github.com/apache/tvm/pull/10686#discussion_r831382689
##########
File path: tests/python/unittest/test_auto_scheduler_feature.py
##########
@@ -220,12 +220,15 @@ def tir_matmul(
def test_primfunc():
- features = auto_scheduler.feature.named_features_from_primfunc(tir_matmul)
Review comment:
Great, seems my interpretation aligns with what you mean. I agree that having a test that thread through lowered is useful if we bring in future usecases that applies feature extractor on lowered code.
To summarize the recommendation:
- Keep the original `test_primfunc`(perhaps rename to `test_prim_func_before_lowering` to clairfy the intend)
- This was the original intended behavior of the extractor and auto-scheduler only relied on this behavior.
- Add a new test `test_primfunc_lowered_with_let`
- I understand that there is an exploration effort to try out this and possibly followup features that depends on this. Although auto-scheduler do not yet rely on this behavior, so it would be good to keep it separate.
- Like you said it is more like an integration tests(but i agree in this case it is OK to keep it here)
- (Optional): add a unit-test testcase with explicitly constructed tvmscript that covers the `lowered_let`. The main rationale is that the intend of the unit-test case would be clear and it becomes easier to debug in facing regression, but I feel in this case it can be optional.
--
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 change in pull request #10686: [AUTO_SCHEDULER] Supported CSE (variable definitions) in feature extraction
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #10686:
URL: https://github.com/apache/tvm/pull/10686#discussion_r831388148
##########
File path: src/auto_scheduler/feature.cc
##########
@@ -233,14 +233,9 @@ AnnotationPosType GetAnnotationPosEncoding(const Var& var, const Array<PrimExpr>
}
}
-// Return the extent of a for loop
-int64_t GetLoopExtent(const ForNode* node) {
- auto pint = node->extent.as<IntImmNode>();
- if (pint != nullptr) {
- return pint->value;
- } else {
- return 1;
- }
+// Return the maximum extent of a for loop
+int64_t GetLoopExtent(const ForNode* node, const Analyzer& ana) {
Review comment:
This LGTM. although I do not expect regression in this case, would be good to run some tests to confirm, not a blocker to merge 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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tkonolige commented on a change in pull request #10686: [AUTO_SCHEDULER] Supported CSE (variable definitions) in feature extraction
Posted by GitBox <gi...@apache.org>.
tkonolige commented on a change in pull request #10686:
URL: https://github.com/apache/tvm/pull/10686#discussion_r831315548
##########
File path: src/auto_scheduler/feature.cc
##########
@@ -233,14 +233,9 @@ AnnotationPosType GetAnnotationPosEncoding(const Var& var, const Array<PrimExpr>
}
}
-// Return the extent of a for loop
-int64_t GetLoopExtent(const ForNode* node) {
- auto pint = node->extent.as<IntImmNode>();
- if (pint != nullptr) {
- return pint->value;
- } else {
- return 1;
- }
+// Return the maximum extent of a for loop
+int64_t GetLoopExtent(const ForNode* node, const Analyzer& ana) {
+ return ana.const_int_bound(node->extent)->max_value;
Review comment:
auto scheduler doesn't work with smbolic shapes, so why would this be a problem? Seems like this is strictly an improvement over what we currently have.
##########
File path: src/auto_scheduler/feature.cc
##########
@@ -233,14 +233,9 @@ AnnotationPosType GetAnnotationPosEncoding(const Var& var, const Array<PrimExpr>
}
}
-// Return the extent of a for loop
-int64_t GetLoopExtent(const ForNode* node) {
- auto pint = node->extent.as<IntImmNode>();
- if (pint != nullptr) {
- return pint->value;
- } else {
- return 1;
- }
+// Return the maximum extent of a for loop
+int64_t GetLoopExtent(const ForNode* node, const Analyzer& ana) {
+ return ana.const_int_bound(node->extent)->max_value;
Review comment:
auto scheduler doesn't work with symbolic shapes, so why would this be a problem? Seems like this is strictly an improvement over what we currently have.
--
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] junrushao1994 commented on pull request #10686: [AUTO_SCHEDULER] Supported CSE (variable definitions) in feature extraction
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on pull request #10686:
URL: https://github.com/apache/tvm/pull/10686#issuecomment-1081120056
CC @tqchen for a second look
--
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] junrushao1994 commented on a change in pull request #10686: [AUTO_SCHEDULER] Supported CSE (variable definitions) in feature extraction
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on a change in pull request #10686:
URL: https://github.com/apache/tvm/pull/10686#discussion_r830509216
##########
File path: src/auto_scheduler/feature.cc
##########
@@ -724,6 +731,15 @@ class PerStoreFeatureExtractor : public StmtExprVisitor {
ExtractAllocationFeature(node);
}
+ void VisitStmt_(const LetStmtNode* node) final {
+ // TODO(tkonolige): add arithmetic counts from this statement to counts of inner stores.
+ ana_.Bind(node->var, node->value);
+ ICHECK(variable_definition_stack_.size() > 0)
+ << "Variable definition out size of a for loop is not handled by feature extraction";
+ variable_definition_stack_.back().push_back(std::make_tuple(node->var, node->value));
+ StmtExprVisitor::VisitStmt_(node);
Review comment:
Do we want to pop after visiting?
##########
File path: include/tvm/arith/analyzer.h
##########
@@ -407,11 +407,13 @@ class TVM_DLL Analyzer {
* \brief Notify all the sub-analyzers that var
* is created and binded to expr.
*
- * Each var can only be binded once.
+ * Each var can only be bound once.
*
* \param var The variable.
* \param expr The expression we bind to.
- * \param allow_override Whether we allow overriding an existing var's expression.
+ * \param allow_override Whether we allow overriding an existing var's
+ * expression. This option should not be used if there is any dependency
Review comment:
Thanks for clarifying this!
##########
File path: src/auto_scheduler/feature.cc
##########
@@ -233,14 +233,9 @@ AnnotationPosType GetAnnotationPosEncoding(const Var& var, const Array<PrimExpr>
}
}
-// Return the extent of a for loop
-int64_t GetLoopExtent(const ForNode* node) {
- auto pint = node->extent.as<IntImmNode>();
- if (pint != nullptr) {
- return pint->value;
- } else {
- return 1;
- }
+// Return the maximum extent of a for loop
+int64_t GetLoopExtent(const ForNode* node, const Analyzer& ana) {
+ return ana.const_int_bound(node->extent)->max_value;
Review comment:
The reason that we didn't use const-int-bound in feature extraction is that it becomes less helpful when encountering TIR of symbolic shapes.
##########
File path: tests/python/unittest/test_auto_scheduler_feature.py
##########
@@ -220,12 +220,15 @@ def tir_matmul(
def test_primfunc():
- features = auto_scheduler.feature.named_features_from_primfunc(tir_matmul)
Review comment:
+1 for TQ's idea
--
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] tkonolige commented on a change in pull request #10686: [AUTO_SCHEDULER] Supported CSE (variable definitions) in feature extraction
Posted by GitBox <gi...@apache.org>.
tkonolige commented on a change in pull request #10686:
URL: https://github.com/apache/tvm/pull/10686#discussion_r831343472
##########
File path: tests/python/unittest/test_auto_scheduler_feature.py
##########
@@ -220,12 +220,15 @@ def tir_matmul(
def test_primfunc():
- features = auto_scheduler.feature.named_features_from_primfunc(tir_matmul)
Review comment:
> To rephrase what you are saying (correct if that was not what you meant):
> - In the future, if CSE changes its behavior to generate other cases, the feature extractor from an explicit tvm-script may not capture that changes (as new CSE will generate new IRs that may have other properties) -- the test won't fail but I guess you mean it fails to capture the possible problem in CSE changes.
What I mean is that I want a test that ensures that changes to the lowering pipeline do not break feature extraction. CSE is an example of a test that broke it. I agree that it could be viewed as an integration test and I can move the test if you'd like.
--
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 change in pull request #10686: [AUTO_SCHEDULER] Supported CSE (variable definitions) in feature extraction
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #10686:
URL: https://github.com/apache/tvm/pull/10686#discussion_r830508197
##########
File path: tests/python/unittest/test_auto_scheduler_feature.py
##########
@@ -220,12 +220,15 @@ def tir_matmul(
def test_primfunc():
- features = auto_scheduler.feature.named_features_from_primfunc(tir_matmul)
Review comment:
Would be good to keep the original pass because that is the original intended usage, since the original function was designed for auto-scheduler with limited passes (to enable fast feature extraction). The test should cover the original intended use.
The additional support enables possible explorations for new use cases. Consider add another testcase test_primfunc_lowered_with_let to get the fully lowered version.
It would be helpful to construct a simplified TVM script version of the lowered version to see exactly where let bound may results in a change of values.
##########
File path: tests/python/unittest/test_auto_scheduler_feature.py
##########
@@ -220,12 +220,15 @@ def tir_matmul(
def test_primfunc():
- features = auto_scheduler.feature.named_features_from_primfunc(tir_matmul)
Review comment:
Would be good to keep the original test because that is the original intended usage, since the original function was designed for auto-scheduler with limited passes (to enable fast feature extraction). The test should cover the original intended use.
The additional support enables possible explorations for new use cases. Consider add another testcase test_primfunc_lowered_with_let to get the fully lowered version.
It would be helpful to construct a simplified TVM script version of the lowered version to see exactly where let bound may results in a change of values.
--
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 change in pull request #10686: [AUTO_SCHEDULER] Supported CSE (variable definitions) in feature extraction
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #10686:
URL: https://github.com/apache/tvm/pull/10686#discussion_r831328067
##########
File path: tests/python/unittest/test_auto_scheduler_feature.py
##########
@@ -220,12 +220,15 @@ def tir_matmul(
def test_primfunc():
- features = auto_scheduler.feature.named_features_from_primfunc(tir_matmul)
Review comment:
I see, explicitly constructed script would help to clarify the regression that the change intend to cover.
To rephrase what you are saying (correct if that was not what you meant):
- In the future, if CSE changes its behavior to generate other cases, the feature extractor from an explicit tvm-script may not capture that changes (as new CSE will generate new IRs that may have other properties) -- the test won't fail but I guess you mean it fails to capture the possible problem in CSE changes.
This is indeed a fair pt. Although running things through is a bit like integration tests(that tests the combined effect of CSE, lowering and feature extraction).
The case of tvmscript inputs is closer to unit-test to test the intended behavior of the specific pass. Such unit-tests usually have a clear intend, and easier to debug when it triggers a regression. Although its coverage won't move as the integration tests do.
I still would suggest keep `test_primfunc` given the original intended behavior of the pass was limited. It does not hurt to have a case to cover the original intended case.
Adding another `test_primfunc_lowered_with_let` would give quite clear context, and provide some safety net when we start to explore the ability of using the extractor with CSE passes.
As for the unit-test case, it is merely a recommendation(as per above discussion on unit-test and integration tests)
--
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 change in pull request #10686: [AUTO_SCHEDULER] Supported CSE (variable definitions) in feature extraction
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #10686:
URL: https://github.com/apache/tvm/pull/10686#discussion_r831388148
##########
File path: src/auto_scheduler/feature.cc
##########
@@ -233,14 +233,9 @@ AnnotationPosType GetAnnotationPosEncoding(const Var& var, const Array<PrimExpr>
}
}
-// Return the extent of a for loop
-int64_t GetLoopExtent(const ForNode* node) {
- auto pint = node->extent.as<IntImmNode>();
- if (pint != nullptr) {
- return pint->value;
- } else {
- return 1;
- }
+// Return the maximum extent of a for loop
+int64_t GetLoopExtent(const ForNode* node, const Analyzer& ana) {
Review comment:
This LGTM. although I do not expect regression in this case, would be good to run some tests to confirm, not a blocker to merge though. cc @junrushao1994
--
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