[Relay] Refactor constant folding over expr into a utility function #13343
Conversation
|
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 by @-ing them in a comment.
Generated by tvm-bot |
| 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); |
There was a problem hiding this comment.
IMHO this keeps the transformation logic a lot cleaner, and allows future reuse.
|
Since it's just a refactor, CI should suffice for testing. |
src/relay/transforms/pattern_utils.h
Outdated
| const_mod = transform::FoldConstant()(const_mod); | ||
| GlobalVar const_main = const_mod->GetGlobalVar("main"); | ||
| return Downcast<Function>(const_mod->functions[const_main])->body; | ||
| } |
There was a problem hiding this comment.
There is already
tvm/src/relay/transforms/fold_constant.cc
Line 429 in 78142ad
And it's odd to add this function in pattern_utils.h.
There was a problem hiding this 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 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. 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:).
There was a problem hiding this comment.
Okay how about this:
- Introduce
fold_constant.hand declareFoldConstantExpr(const Expr& expr)andFoldConstantExpr(const Expr& expr, const IRModule& mod, bool fold_qnn). - Implement the former one in terms of the latter.
- Remove all duplicated definitions of
FoldConstantExprand use the new ones above.
There was a problem hiding this comment.
I like that, done!
Also caught another duplication in relay/quantize/realize.cc. Let me know if this PR needs more tweaking.
StrongerXi
force-pushed
the
refactor-relay-constant-fold-expr-logic
branch
from
0a58854 to
fb22a27
Compare
| [](const Expr& expr, const IRModule& mod, bool fold_qnn) { | ||
| return FoldConstantExpr(expr, mod, fold_qnn); | ||
| } | ||
| ); |
There was a problem hiding this comment.
This is needed to resolve the name overloading -- I decided to keep the FoldConstantExpr names.
StrongerXi
force-pushed
the
refactor-relay-constant-fold-expr-logic
branch
from
fb22a27 to
479a78a
Compare
|
Make linter happy. |
StrongerXi
force-pushed
the
refactor-relay-constant-fold-expr-logic
branch
from
479a78a to
bb67e29
Compare
|
Fix build failure in |
4 participants
Refactor duplicated and low-level logic for applying constant-folding over an
Exprinto a utility function.