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/11/10 03:30:22 UTC
[GitHub] [tvm] StrongerXi opened a new pull request, #13343: [Relay] Refactor constant folding over expr into a utility function
StrongerXi opened a new pull request, #13343:
URL: https://github.com/apache/tvm/pull/13343
Refactor duplicated and low-level logic for applying constant-folding over an `Expr` into a utility function.
--
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 #13343: [Relay] Refactor constant folding over expr into a utility function
Posted by GitBox <gi...@apache.org>.
tvm-bot commented on PR #13343:
URL: https://github.com/apache/tvm/pull/13343#issuecomment-1309720896
<!---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 tag found in teams: `relay` <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 commented on a diff in pull request #13343: [Relay] Refactor constant folding over expr into a utility function
Posted by GitBox <gi...@apache.org>.
masahi commented on code in PR #13343:
URL: https://github.com/apache/tvm/pull/13343#discussion_r1018792828
##########
src/relay/transforms/pattern_utils.h:
##########
@@ -120,6 +120,21 @@ inline Expr InferType(const Expr& expr) {
}
}
+/*!
+ * \brief Try to apply constant folding over expr:
+ *
+ * Do constant_fold over each node in expr
+ *
+ * \param expr The IR expression
+ * \return resulting expr
+ */
+inline Expr FoldConstantExpr(const Expr& expr) {
+ IRModule const_mod = IRModule::FromExpr(expr);
+ const_mod = transform::FoldConstant()(const_mod);
+ GlobalVar const_main = const_mod->GetGlobalVar("main");
+ return Downcast<Function>(const_mod->functions[const_main])->body;
+}
Review Comment:
There is already https://github.com/apache/tvm/blob/78142ad2e4ce2c5ca096f7e80f4f4f62ca9c2bd5/src/relay/transforms/fold_constant.cc#L429. You can probably just use that.
And it's odd to add this function in `pattern_utils.h`.
--
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] StrongerXi commented on a diff in pull request #13343: [Relay] Refactor constant folding over expr into a utility function
Posted by GitBox <gi...@apache.org>.
StrongerXi commented on code in PR #13343:
URL: https://github.com/apache/tvm/pull/13343#discussion_r1019722762
##########
src/relay/transforms/pattern_utils.h:
##########
@@ -120,6 +120,21 @@ inline Expr InferType(const Expr& expr) {
}
}
+/*!
+ * \brief Try to apply constant folding over expr:
+ *
+ * Do constant_fold over each node in expr
+ *
+ * \param expr The IR expression
+ * \return resulting expr
+ */
+inline Expr FoldConstantExpr(const Expr& expr) {
+ IRModule const_mod = IRModule::FromExpr(expr);
+ const_mod = transform::FoldConstant()(const_mod);
+ GlobalVar const_main = const_mod->GetGlobalVar("main");
+ return Downcast<Function>(const_mod->functions[const_main])->body;
+}
Review Comment:
I like that, done!
Caught another duplication in `relay/quantize/realize.cc`. Let me know if it needs more tweaking.
--
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 #13343: [Relay] Refactor constant folding over expr into a utility function
Posted by GitBox <gi...@apache.org>.
masahi commented on code in PR #13343:
URL: https://github.com/apache/tvm/pull/13343#discussion_r1019638394
##########
src/relay/transforms/pattern_utils.h:
##########
@@ -120,6 +120,21 @@ inline Expr InferType(const Expr& expr) {
}
}
+/*!
+ * \brief Try to apply constant folding over expr:
+ *
+ * Do constant_fold over each node in expr
+ *
+ * \param expr The IR expression
+ * \return resulting expr
+ */
+inline Expr FoldConstantExpr(const Expr& expr) {
+ IRModule const_mod = IRModule::FromExpr(expr);
+ const_mod = transform::FoldConstant()(const_mod);
+ GlobalVar const_main = const_mod->GetGlobalVar("main");
+ return Downcast<Function>(const_mod->functions[const_main])->body;
+}
Review Comment:
Okay how about this:
* Introduce `fold_constant.h` and declare `FoldConstantExpr(const Expr& expr)` and `FoldConstantExpr(const Expr& expr, const IRModule& mod, bool fold_qnn)`.
* Implement the former one in terms of the latter.
* Replace all duplicated definition of `FoldConstantExpr` with these ones.
--
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 pull request #13343: [Relay] Refactor constant folding over expr into a utility function
Posted by GitBox <gi...@apache.org>.
masahi commented on PR #13343:
URL: https://github.com/apache/tvm/pull/13343#issuecomment-1311244594
1.
--
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] StrongerXi commented on pull request #13343: [Relay] Refactor constant folding over expr into a utility function
Posted by GitBox <gi...@apache.org>.
StrongerXi commented on PR #13343:
URL: https://github.com/apache/tvm/pull/13343#issuecomment-1311157017
Fix build failure in `contrib`, which wasn't triggered locally from default build.
--
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] StrongerXi commented on a diff in pull request #13343: [Relay] Refactor constant folding over expr into a utility function
Posted by GitBox <gi...@apache.org>.
StrongerXi commented on code in PR #13343:
URL: https://github.com/apache/tvm/pull/13343#discussion_r1019722762
##########
src/relay/transforms/pattern_utils.h:
##########
@@ -120,6 +120,21 @@ inline Expr InferType(const Expr& expr) {
}
}
+/*!
+ * \brief Try to apply constant folding over expr:
+ *
+ * Do constant_fold over each node in expr
+ *
+ * \param expr The IR expression
+ * \return resulting expr
+ */
+inline Expr FoldConstantExpr(const Expr& expr) {
+ IRModule const_mod = IRModule::FromExpr(expr);
+ const_mod = transform::FoldConstant()(const_mod);
+ GlobalVar const_main = const_mod->GetGlobalVar("main");
+ return Downcast<Function>(const_mod->functions[const_main])->body;
+}
Review Comment:
I like that, done!
Also caught another duplication in `relay/quantize/realize.cc`. Let me know if this PR needs more tweaking.
--
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] StrongerXi commented on pull request #13343: [Relay] Refactor constant folding over expr into a utility function
Posted by GitBox <gi...@apache.org>.
StrongerXi commented on PR #13343:
URL: https://github.com/apache/tvm/pull/13343#issuecomment-1311126603
Make linter happy.
--
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] StrongerXi commented on pull request #13343: [Relay] Refactor constant folding over expr into a utility function
Posted by GitBox <gi...@apache.org>.
StrongerXi commented on PR #13343:
URL: https://github.com/apache/tvm/pull/13343#issuecomment-1309721517
Since it's just a refactor, CI should suffice for testing.
--
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] StrongerXi commented on a diff in pull request #13343: [Relay] Refactor constant folding over expr into a utility function
Posted by GitBox <gi...@apache.org>.
StrongerXi commented on code in PR #13343:
URL: https://github.com/apache/tvm/pull/13343#discussion_r1018619124
##########
src/relay/transforms/simplify_expr.cc:
##########
@@ -795,10 +795,7 @@ class SwitchAddMultiply : public DFPatternRewrite {
}
Expr const_expr = Call(Op::Get("multiply"), {c1, c2});
- IRModule const_mod = IRModule::FromExpr(const_expr);
- const_mod = transform::FoldConstant()(const_mod);
- GlobalVar const_main = const_mod->GetGlobalVar("main");
- Expr const_val = Downcast<Function>(const_mod->functions[const_main])->body;
+ Expr const_val = FoldConstantExpr(const_expr);
Review Comment:
IMHO this keeps the transformation logic a lot cleaner, and allows future reuse.
--
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] StrongerXi commented on a diff in pull request #13343: [Relay] Refactor constant folding over expr into a utility function
Posted by GitBox <gi...@apache.org>.
StrongerXi commented on code in PR #13343:
URL: https://github.com/apache/tvm/pull/13343#discussion_r1019253744
##########
src/relay/transforms/pattern_utils.h:
##########
@@ -120,6 +120,21 @@ inline Expr InferType(const Expr& expr) {
}
}
+/*!
+ * \brief Try to apply constant folding over expr:
+ *
+ * Do constant_fold over each node in expr
+ *
+ * \param expr The IR expression
+ * \return resulting expr
+ */
+inline Expr FoldConstantExpr(const Expr& expr) {
+ IRModule const_mod = IRModule::FromExpr(expr);
+ const_mod = transform::FoldConstant()(const_mod);
+ GlobalVar const_main = const_mod->GetGlobalVar("main");
+ return Downcast<Function>(const_mod->functions[const_main])->body;
+}
Review Comment:
> You can probably just use that.
Thanks, I did see that one, but (1). currently it doesn't seem to be declared in any header, and (2). the extra `mod` argument is not ergonomic for our use case here.
> And it's odd to add this function in pattern_utils.h.
I agree, I did this for consistency because we already have [InferType](https://github.com/apache/tvm/blob/main/src/relay/transforms/pattern_utils.h#L113-L121) right above from #13275.
I think they should probably both be moved to either `pass_utils.h` or a new `transform_utils.h`.
Regardless, I also just found [this](https://github.com/apache/tvm/blob/main/src/relay/backend/contrib/constant_transforms.h#L36-L43). It's in `contrib`, but if we are okay with using it I'll go ahead.
Let me know what you think, I'll update the PR accordingly:).
--
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 #13343: [Relay] Refactor constant folding over expr into a utility function
Posted by GitBox <gi...@apache.org>.
masahi commented on code in PR #13343:
URL: https://github.com/apache/tvm/pull/13343#discussion_r1019638394
##########
src/relay/transforms/pattern_utils.h:
##########
@@ -120,6 +120,21 @@ inline Expr InferType(const Expr& expr) {
}
}
+/*!
+ * \brief Try to apply constant folding over expr:
+ *
+ * Do constant_fold over each node in expr
+ *
+ * \param expr The IR expression
+ * \return resulting expr
+ */
+inline Expr FoldConstantExpr(const Expr& expr) {
+ IRModule const_mod = IRModule::FromExpr(expr);
+ const_mod = transform::FoldConstant()(const_mod);
+ GlobalVar const_main = const_mod->GetGlobalVar("main");
+ return Downcast<Function>(const_mod->functions[const_main])->body;
+}
Review Comment:
Okay how about this:
* Introduce `fold_constant.h` and declare `FoldConstantExpr(const Expr& expr)` and `FoldConstantExpr(const Expr& expr, const IRModule& mod, bool fold_qnn)`.
* Implement the former one in terms of the latter.
* Remove all duplicated definitions of `FoldConstantExpr` and use the new ones above.
--
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] StrongerXi commented on a diff in pull request #13343: [Relay] Refactor constant folding over expr into a utility function
Posted by GitBox <gi...@apache.org>.
StrongerXi commented on code in PR #13343:
URL: https://github.com/apache/tvm/pull/13343#discussion_r1019723407
##########
src/relay/transforms/fold_constant.cc:
##########
@@ -434,11 +426,20 @@ Expr FoldConstantExpr(const Expr& expr, const IRModule& mod, bool fold_qnn) {
return result;
}
-TVM_REGISTER_GLOBAL("relay._transform.FoldConstantExpr").set_body_typed(FoldConstantExpr);
+Expr FoldConstantExpr(const Expr& expr, bool fold_qnn) {
+ auto mod = IRModule::FromExpr(expr);
+ return FoldConstantExpr(expr, mod, fold_qnn);
+}
+
+TVM_REGISTER_GLOBAL("relay._transform.FoldConstantExpr").set_body_typed(
+ [](const Expr& expr, const IRModule& mod, bool fold_qnn) {
+ return FoldConstantExpr(expr, mod, fold_qnn);
+ }
+);
Review Comment:
Needed to resolve the name overloading. I decided to keep the `FoldConstantExpr` names.
--
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 #13343: [Relay] Refactor constant folding over expr into a utility function
Posted by GitBox <gi...@apache.org>.
masahi merged PR #13343:
URL: https://github.com/apache/tvm/pull/13343
--
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] StrongerXi commented on a diff in pull request #13343: [Relay] Refactor constant folding over expr into a utility function
Posted by GitBox <gi...@apache.org>.
StrongerXi commented on code in PR #13343:
URL: https://github.com/apache/tvm/pull/13343#discussion_r1019723407
##########
src/relay/transforms/fold_constant.cc:
##########
@@ -434,11 +426,20 @@ Expr FoldConstantExpr(const Expr& expr, const IRModule& mod, bool fold_qnn) {
return result;
}
-TVM_REGISTER_GLOBAL("relay._transform.FoldConstantExpr").set_body_typed(FoldConstantExpr);
+Expr FoldConstantExpr(const Expr& expr, bool fold_qnn) {
+ auto mod = IRModule::FromExpr(expr);
+ return FoldConstantExpr(expr, mod, fold_qnn);
+}
+
+TVM_REGISTER_GLOBAL("relay._transform.FoldConstantExpr").set_body_typed(
+ [](const Expr& expr, const IRModule& mod, bool fold_qnn) {
+ return FoldConstantExpr(expr, mod, fold_qnn);
+ }
+);
Review Comment:
This is needed to resolve the name overloading -- I decided to keep the `FoldConstantExpr` names.
--
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] StrongerXi commented on a diff in pull request #13343: [Relay] Refactor constant folding over expr into a utility function
Posted by GitBox <gi...@apache.org>.
StrongerXi commented on code in PR #13343:
URL: https://github.com/apache/tvm/pull/13343#discussion_r1019253744
##########
src/relay/transforms/pattern_utils.h:
##########
@@ -120,6 +120,21 @@ inline Expr InferType(const Expr& expr) {
}
}
+/*!
+ * \brief Try to apply constant folding over expr:
+ *
+ * Do constant_fold over each node in expr
+ *
+ * \param expr The IR expression
+ * \return resulting expr
+ */
+inline Expr FoldConstantExpr(const Expr& expr) {
+ IRModule const_mod = IRModule::FromExpr(expr);
+ const_mod = transform::FoldConstant()(const_mod);
+ GlobalVar const_main = const_mod->GetGlobalVar("main");
+ return Downcast<Function>(const_mod->functions[const_main])->body;
+}
Review Comment:
> You can probably just use that.
Thanks, I did see that one, but (1). currently it doesn't seem to be declared in any header, and (2). the extra `mod` argument is not ergonomic for our use case here.
> And it's odd to add this function in pattern_utils.h.
I agree, I did this for consistency because we had [InferType](https://github.com/apache/tvm/blob/main/src/relay/transforms/pattern_utils.h#L113-L121) right above from #13275.
I think they should probably both be moved to either `pass_utils.h` or a new `transform_utils.h`.
Let me know what you think, I'll update the PR accordingly:).
##########
src/relay/transforms/pattern_utils.h:
##########
@@ -120,6 +120,21 @@ inline Expr InferType(const Expr& expr) {
}
}
+/*!
+ * \brief Try to apply constant folding over expr:
+ *
+ * Do constant_fold over each node in expr
+ *
+ * \param expr The IR expression
+ * \return resulting expr
+ */
+inline Expr FoldConstantExpr(const Expr& expr) {
+ IRModule const_mod = IRModule::FromExpr(expr);
+ const_mod = transform::FoldConstant()(const_mod);
+ GlobalVar const_main = const_mod->GetGlobalVar("main");
+ return Downcast<Function>(const_mod->functions[const_main])->body;
+}
Review Comment:
> You can probably just use that.
Thanks, I did see that one, but (1). currently it doesn't seem to be declared in any header, and (2). the extra `mod` argument is not ergonomic for our use case here.
> And it's odd to add this function in pattern_utils.h.
I agree, I did this for consistency because we already have [InferType](https://github.com/apache/tvm/blob/main/src/relay/transforms/pattern_utils.h#L113-L121) right above from #13275.
I think they should probably both be moved to either `pass_utils.h` or a new `transform_utils.h`.
Let me know what you think, I'll update the PR accordingly:).
--
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] StrongerXi commented on a diff in pull request #13343: [Relay] Refactor constant folding over expr into a utility function
Posted by GitBox <gi...@apache.org>.
StrongerXi commented on code in PR #13343:
URL: https://github.com/apache/tvm/pull/13343#discussion_r1019253744
##########
src/relay/transforms/pattern_utils.h:
##########
@@ -120,6 +120,21 @@ inline Expr InferType(const Expr& expr) {
}
}
+/*!
+ * \brief Try to apply constant folding over expr:
+ *
+ * Do constant_fold over each node in expr
+ *
+ * \param expr The IR expression
+ * \return resulting expr
+ */
+inline Expr FoldConstantExpr(const Expr& expr) {
+ IRModule const_mod = IRModule::FromExpr(expr);
+ const_mod = transform::FoldConstant()(const_mod);
+ GlobalVar const_main = const_mod->GetGlobalVar("main");
+ return Downcast<Function>(const_mod->functions[const_main])->body;
+}
Review Comment:
> You can probably just use that.
Thanks, I did see that one, but (1). currently it doesn't seem to be declared in any header, and (2). the extra `mod` argument is not ergonomic for our use case here.
> And it's odd to add this function in pattern_utils.h.
I agree, I did this for consistency because we had [InferType](https://github.com/apache/tvm/blob/main/src/relay/transforms/pattern_utils.h#L113-L121) right above from #13275.
I think they should probably both be moved to either `pass_utils.h` or a new `transform_utils.h`.
Let me know what you think, I'll update the PR accordingly:).
--
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