You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by "Lunderberg (via GitHub)" <gi...@apache.org> on 2023/11/03 18:10:56 UTC
[PR] [Unity] Implement FNormalize attribute for operators [tvm]
Lunderberg opened a new pull request, #16067:
URL: https://github.com/apache/tvm/pull/16067
Some Relax operators have requirements regarding their AST that are stronger than are checked by the C++ types being used. These are similar to checks that are present in the `tvm::relax::WellFormed` utility, such as checks forbidding the use of undefined variables, which are also stronger than required by the underlying C++ types. However, because every operator may have unique requirements, it would be unreasonable to expect a writer of a `relax::ExprMutator` to be aware of and to maintain all such requirements.
This PR introduces an operation operator attribute `FNormalize`. If defined, this function is used to apply an operator-specific normalization. The implementation of `FNormalize` has the following design decisions.
* If no change is required, `FNormalize` should return the input argument unmodified.
* `FNormalize` is only responsible for normalization of the operator itself. The expression it returns may be unnormalized (e.g. contain nested expressions).
* `FNormalize` receives the `BlockBuilder` as an argument, to allow context-dependent normalization.
For example, an operator whose normalization requires in-line expressions may use `BlockBuilder::LookupBinding` to perform variable replacement.
* `FNormalize` is applied after `FInferStructInfo`. `FNormalize` may assume that the `relax::Call` passed to `FNormalize` has well-defined struct info.
* Corollary: `FInferStructInfo` may not assume that its `relax::Call` argument has been passed through `FNormalize`.
This is a reasonable requirement, because (1) shape inference should depend only on the struct info of arguments and not the values themselves, and (2) this only impacts operators that use `FNormalize`.
* `FNormalize` should not be used to apply simplifications, and should be limited to cases where the same computation may be expressed in multiple manners.
For example, replacing a by-variable tuple with an in-line tuple in `R.call_tir` is a form of normalization, but replacing `R.add(arg, R.const(0))` with `arg` is a form of simplification.
This separation is to ensure that `FNormalize` has minimal overhead, as some simplifications may have large computational costs, and `FNormalize` is applied as part of all `ExprMutator` usage. A later PR will introduce an attribute `FSimplify`, along with a dedicated pass to apply simplifications.
* Use of `FNormalize` is suppressed while parsing TVMScript. TVMScript must be able to generate test cases that trigger specific failure modes, and that may include producing un-normalized relax IR. In addition, TVMScript must be stable when passed through a round-trip from IR to text to IR.
--
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
Re: [PR] [Unity] Implement FNormalize attribute for operators [tvm]
Posted by "Lunderberg (via GitHub)" <gi...@apache.org>.
Lunderberg commented on code in PR #16067:
URL: https://github.com/apache/tvm/pull/16067#discussion_r1383694728
##########
src/relax/ir/block_builder.cc:
##########
@@ -600,6 +596,27 @@ class Normalizer : public BlockBuilderImpl, private ExprFunctor<Expr(const Expr&
UpdateStructInfo(call, inferred_sinfo);
}
+ // In the spirit of joy and child-like whimsy, the conditional
+ // statement below is provided in both C++ and tongue-twister
+ // form. As very few C++ compilers are fluent in tongue-twister,
+ // only the C++ form is uncommented.
+ //
+ // How much opt could an opt op Op if an opt op could op opt?
+ if (auto opt_op = op->op.as<Op>()) {
+ auto op = opt_op.value();
+ if (apply_f_normalize_ && op_map_normalize_.count(op)) {
Review Comment:
Thank you, and updated! I had checked for a single-parameter `.get`, and an iterator-style `.find`, but hadn't found the two-parameter `.get`.
Updated to use `if (auto func_normalize = op_map_normalize_.get(call->op, nullptr); func_normalize != nullptr)`, here and in `well_formed.cc`.
--
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
Re: [PR] [Unity] Implement FNormalize attribute for operators [tvm]
Posted by "tqchen (via GitHub)" <gi...@apache.org>.
tqchen commented on PR #16067:
URL: https://github.com/apache/tvm/pull/16067#issuecomment-1794967438
Thanks for the proposed change. I like how FNormalize can help reducing overhead of creating certain operators and bring them back to normal form.
I only have one comment on the wellform check side. In this case, it is useful to have an intentionally duplicated check that is different from FNormalize , e.g. have a `TEnforceExplicitTupleInArgs` attribute that enforces the tuple argument being unpacked, and check this condition. This provides extra layer of protection, makes the intention clear and is also more efficient
--
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
Re: [PR] [Unity] Implement FNormalize attribute for operators [tvm]
Posted by "Lunderberg (via GitHub)" <gi...@apache.org>.
Lunderberg commented on PR #16067:
URL: https://github.com/apache/tvm/pull/16067#issuecomment-1795153381
Thank you, and I like the overall design. I think we still want to keep all the normalization logic in `FNormalize`, without adding boolean flags for specific cases. The more boolean flags we have, the more difficult it is for a developer to know the rules for all flags. For example, a developer would need to check if the `TEnforceExplicitTupleInArgs` is used as part of the well-formed check, whether it triggers an assert during normalization, whether it triggers a normalization step during normalization, whether the normalization step applies when parsing TVMScript, etc. By implementing each of these features on top of the same `FNormalize` functionality, new operator-specific normalization rules can be implemented without adding to a developer's mental overhead. A developer only needs to know that the new normalization is handled the same as all existing integrations.
(Also, see the other comment for performance benchmarking.)
--
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
Re: [PR] [Unity] Implement FNormalize attribute for operators [tvm]
Posted by "Lunderberg (via GitHub)" <gi...@apache.org>.
Lunderberg commented on code in PR #16067:
URL: https://github.com/apache/tvm/pull/16067#discussion_r1383511660
##########
src/relax/analysis/well_formed.cc:
##########
@@ -301,13 +302,38 @@ class WellFormedChecker : public relax::ExprVisitor,
}
}
- for (const StructInfo& sinfo_arg : op->sinfo_args) {
+ for (const StructInfo& sinfo_arg : call->sinfo_args) {
this->VisitStructInfo(sinfo_arg);
}
- CheckStructInfo(op);
- if (is_dataflow_ && check_struct_info_ && IsImpureCall(GetRef<Call>(op))) {
- Malformed(Diagnostic::Error(op) << "There cannot be an impure call inside a dataflow block.");
+ CheckStructInfo(call);
+ if (is_dataflow_ && check_struct_info_ && IsImpureCall(GetRef<Call>(call))) {
+ Malformed(Diagnostic::Error(call)
+ << "There cannot be an impure call inside a dataflow block.");
+ }
+
+ // If the operation has defined a custom normalization function
+ // using the FNormalize attribute, the call node must be normalized in order to be well-formed.
Review Comment:
> While this is general. It also comes with overhead of creating a BlockBuilder.
I did a few benchmarks to verify, and the creation of a `BlockBuilder` has negligible overhead. The overhead of constructing it is about 1 microsecond. Measuring this requiring removing several larger overheads, including the `LOG(DEBUG)` on ill-formed output (6 microseconds), and the overhead of passing the error message through the PackedFunc interface (~50 microseconds).
Since the overhead is negligible, we can avoid duplication of the same checks by implementing the well-formed check in terms of normalization. That is, being well-formed requires being normalized.
--
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
Re: [PR] [Unity] Implement FNormalize attribute for operators [tvm]
Posted by "tqchen (via GitHub)" <gi...@apache.org>.
tqchen commented on code in PR #16067:
URL: https://github.com/apache/tvm/pull/16067#discussion_r1383609933
##########
src/relax/analysis/well_formed.cc:
##########
@@ -301,13 +302,38 @@ class WellFormedChecker : public relax::ExprVisitor,
}
}
- for (const StructInfo& sinfo_arg : op->sinfo_args) {
+ for (const StructInfo& sinfo_arg : call->sinfo_args) {
this->VisitStructInfo(sinfo_arg);
}
- CheckStructInfo(op);
- if (is_dataflow_ && check_struct_info_ && IsImpureCall(GetRef<Call>(op))) {
- Malformed(Diagnostic::Error(op) << "There cannot be an impure call inside a dataflow block.");
+ CheckStructInfo(call);
+ if (is_dataflow_ && check_struct_info_ && IsImpureCall(GetRef<Call>(call))) {
+ Malformed(Diagnostic::Error(call)
+ << "There cannot be an impure call inside a dataflow block.");
+ }
+
+ // If the operation has defined a custom normalization function
+ // using the FNormalize attribute, the call node must be normalized in order to be well-formed.
+ // If we apply the FNormalize and it produces any change, modified the expression, re-visit in
+ // case it produced a nested expression.
+
+ if (auto opt_op = call->op.as<Op>()) {
+ auto op = opt_op.value();
Review Comment:
op_map_normalize_.get(call->op, nullptr)
--
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
Re: [PR] [Unity] Implement FNormalize attribute for operators [tvm]
Posted by "tqchen (via GitHub)" <gi...@apache.org>.
tqchen commented on PR #16067:
URL: https://github.com/apache/tvm/pull/16067#issuecomment-1795347407
after thinking a bit more, i now agree that we can reuse FNormalize in wellform check. thanks for proposing the change
--
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
Re: [PR] [Unity] Implement FNormalize attribute for operators [tvm]
Posted by "Lunderberg (via GitHub)" <gi...@apache.org>.
Lunderberg commented on code in PR #16067:
URL: https://github.com/apache/tvm/pull/16067#discussion_r1383695202
##########
src/relax/analysis/well_formed.cc:
##########
@@ -301,13 +302,38 @@ class WellFormedChecker : public relax::ExprVisitor,
}
}
- for (const StructInfo& sinfo_arg : op->sinfo_args) {
+ for (const StructInfo& sinfo_arg : call->sinfo_args) {
this->VisitStructInfo(sinfo_arg);
}
- CheckStructInfo(op);
- if (is_dataflow_ && check_struct_info_ && IsImpureCall(GetRef<Call>(op))) {
- Malformed(Diagnostic::Error(op) << "There cannot be an impure call inside a dataflow block.");
+ CheckStructInfo(call);
+ if (is_dataflow_ && check_struct_info_ && IsImpureCall(GetRef<Call>(call))) {
+ Malformed(Diagnostic::Error(call)
+ << "There cannot be an impure call inside a dataflow block.");
+ }
+
+ // If the operation has defined a custom normalization function
+ // using the FNormalize attribute, the call node must be normalized in order to be well-formed.
+ // If we apply the FNormalize and it produces any change, modified the expression, re-visit in
+ // case it produced a nested expression.
+
+ if (auto opt_op = call->op.as<Op>()) {
+ auto op = opt_op.value();
Review Comment:
Thank you, and updated to use `if (auto func_normalize = op_map_normalize_.get(call->op, nullptr); func_normalize != nullptr)`.
--
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
Re: [PR] [Unity] Implement FNormalize attribute for operators [tvm]
Posted by "tqchen (via GitHub)" <gi...@apache.org>.
tqchen commented on code in PR #16067:
URL: https://github.com/apache/tvm/pull/16067#discussion_r1383609197
##########
src/relax/analysis/well_formed.cc:
##########
@@ -301,13 +302,38 @@ class WellFormedChecker : public relax::ExprVisitor,
}
}
- for (const StructInfo& sinfo_arg : op->sinfo_args) {
+ for (const StructInfo& sinfo_arg : call->sinfo_args) {
this->VisitStructInfo(sinfo_arg);
}
- CheckStructInfo(op);
- if (is_dataflow_ && check_struct_info_ && IsImpureCall(GetRef<Call>(op))) {
- Malformed(Diagnostic::Error(op) << "There cannot be an impure call inside a dataflow block.");
+ CheckStructInfo(call);
+ if (is_dataflow_ && check_struct_info_ && IsImpureCall(GetRef<Call>(call))) {
+ Malformed(Diagnostic::Error(call)
+ << "There cannot be an impure call inside a dataflow block.");
+ }
+
+ // If the operation has defined a custom normalization function
+ // using the FNormalize attribute, the call node must be normalized in order to be well-formed.
+ // If we apply the FNormalize and it produces any change, modified the expression, re-visit in
+ // case it produced a nested expression.
+
+ if (auto opt_op = call->op.as<Op>()) {
+ auto op = opt_op.value();
Review Comment:
https://github.com/apache/tvm/blob/main/include/tvm/ir/op.h#L476 we can directly use this function to simplofy the logic
--
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
Re: [PR] [Unity] Implement FNormalize attribute for operators [tvm]
Posted by "tqchen (via GitHub)" <gi...@apache.org>.
tqchen commented on code in PR #16067:
URL: https://github.com/apache/tvm/pull/16067#discussion_r1383432921
##########
src/relax/analysis/well_formed.cc:
##########
@@ -301,13 +302,38 @@ class WellFormedChecker : public relax::ExprVisitor,
}
}
- for (const StructInfo& sinfo_arg : op->sinfo_args) {
+ for (const StructInfo& sinfo_arg : call->sinfo_args) {
this->VisitStructInfo(sinfo_arg);
}
- CheckStructInfo(op);
- if (is_dataflow_ && check_struct_info_ && IsImpureCall(GetRef<Call>(op))) {
- Malformed(Diagnostic::Error(op) << "There cannot be an impure call inside a dataflow block.");
+ CheckStructInfo(call);
+ if (is_dataflow_ && check_struct_info_ && IsImpureCall(GetRef<Call>(call))) {
+ Malformed(Diagnostic::Error(call)
+ << "There cannot be an impure call inside a dataflow block.");
+ }
+
+ // If the operation has defined a custom normalization function
+ // using the FNormalize attribute, the call node must be normalized in order to be well-formed.
Review Comment:
While this is general. It also comes with overhead of creating a BlockBuilder.
Instead, it is useful to explicitly spell out the constraints of the related ops. e.g. call_tir do not allow recursive Tuple and we chekc that condition.
This would also add an extra layer of safeguard of FNormalize
--
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
Re: [PR] [Unity] Implement FNormalize attribute for operators [tvm]
Posted by "Lunderberg (via GitHub)" <gi...@apache.org>.
Lunderberg commented on PR #16067:
URL: https://github.com/apache/tvm/pull/16067#issuecomment-1795567680
Thank you, and changes made as suggested!
--
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
Re: [PR] [Unity] Implement FNormalize attribute for operators [tvm]
Posted by "tqchen (via GitHub)" <gi...@apache.org>.
tqchen merged PR #16067:
URL: https://github.com/apache/tvm/pull/16067
--
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
Re: [PR] [Unity] Implement FNormalize attribute for operators [tvm]
Posted by "tqchen (via GitHub)" <gi...@apache.org>.
tqchen commented on code in PR #16067:
URL: https://github.com/apache/tvm/pull/16067#discussion_r1383608693
##########
src/relax/ir/block_builder.cc:
##########
@@ -600,6 +596,27 @@ class Normalizer : public BlockBuilderImpl, private ExprFunctor<Expr(const Expr&
UpdateStructInfo(call, inferred_sinfo);
}
+ // In the spirit of joy and child-like whimsy, the conditional
+ // statement below is provided in both C++ and tongue-twister
+ // form. As very few C++ compilers are fluent in tongue-twister,
+ // only the C++ form is uncommented.
+ //
+ // How much opt could an opt op Op if an opt op could op opt?
+ if (auto opt_op = op->op.as<Op>()) {
+ auto op = opt_op.value();
+ if (apply_f_normalize_ && op_map_normalize_.count(op)) {
Review Comment:
We can use this function https://github.com/apache/tvm/blob/main/include/tvm/ir/op.h#L476
--
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