[BYOC] Support control flow in annotate_target#6641
[BYOC] Support control flow in annotate_target#6641comaniac merged 19 commits intoapache:mainfrom codeislife99:annotate_target
Conversation
|
Hi @codeislife99, thanks for the PR. Would you mind providing a bit more context on what this change is and why is it needed? Also, generally speaking, it would be good to have some test cases added when changes are introduced. That would help a lot, when reviewing your PR. |
|
@leandron @junrushao1994 The PR is now ready for review. Please take a look. |
|
@leandron @junrushao1994 The description is updated. PTAL. |
comaniac
left a comment
comaniac
left a comment
There was a problem hiding this comment.
The logic looks clear to me. Since this PR does not bring anything substantially new but just improve the annotation pass to support control flow related ops (e.g., If), I think we can skip the RFC process.
| return std::move(new_expr); | ||
| } | ||
|
|
||
| Expr InsertCompilerEnd(const Expr& expr) { |
There was a problem hiding this comment.
- Put this function together with
AnnotateArgs. - Add formal function description.
- From its functionality, the name like
InsertCompilerEndAndPropogateTargetwould be more proper. - Use this function in
Expr Rewrite_(const FunctionNode* fn, const Expr& post)to avoid redundant logic.
There was a problem hiding this comment.
Done. Although using this function in AnnotateArgs makes us do the same condition check twice which I think is unnecessary. For the time being I added it, but let me know your thoughts. Maybe I misinterpreted what you meant by Put this function together with AnnotateArgs ?
There was a problem hiding this comment.
Sorry for not being clear. For the last point, I meant you can use InertCompilerEndAndPropogateTarget in https://github.com/apache/incubator-tvm/blob/main/src/relay/transforms/annotate_target.cc#L225 to replace the same logic. This point has nothing to do with AnnotateArgs tho
There was a problem hiding this comment.
Ah I see , oh so thats already done now in the previous commit. I will remove it from AnnotateArgs
comaniac
added
status: review in progress
and removed
status: need RFC
comaniac
changed the title
|
@comaniac I addressed your comments. PTAL again. cc: @anijain2305 |
|
Thanks @codeislife99 |
* Change annotate target * Annotate_target * Revert namespace changes * Add tests for if-else node * Add while_let testcase * No merging in ifelse * Remove scope builder * Add ops * Replace < with less * Linter * Pass Tests * Change back to static const * Cpplinter * address PR comments' * PR Comments * Clang-format check * PR Comments * PR Comments * Change back to Insert Ann in AnnotateARgs Co-authored-by: Ritwik Das <dasritwi@3c22fb14d7c6.ant.amazon.com> Co-authored-by: Ubuntu <ubuntu@ip-172-31-3-223.us-west-2.compute.internal>
* Change annotate target * Annotate_target * Revert namespace changes * Add tests for if-else node * Add while_let testcase * No merging in ifelse * Remove scope builder * Add ops * Replace < with less * Linter * Pass Tests * Change back to static const * Cpplinter * address PR comments' * PR Comments * Clang-format check * PR Comments * PR Comments * Change back to Insert Ann in AnnotateARgs Co-authored-by: Ritwik Das <dasritwi@3c22fb14d7c6.ant.amazon.com> Co-authored-by: Ubuntu <ubuntu@ip-172-31-3-223.us-west-2.compute.internal>
* Change annotate target * Annotate_target * Revert namespace changes * Add tests for if-else node * Add while_let testcase * No merging in ifelse * Remove scope builder * Add ops * Replace < with less * Linter * Pass Tests * Change back to static const * Cpplinter * address PR comments' * PR Comments * Clang-format check * PR Comments * PR Comments * Change back to Insert Ann in AnnotateARgs Co-authored-by: Ritwik Das <dasritwi@3c22fb14d7c6.ant.amazon.com> Co-authored-by: Ubuntu <ubuntu@ip-172-31-3-223.us-west-2.compute.internal>
* Change annotate target * Annotate_target * Revert namespace changes * Add tests for if-else node * Add while_let testcase * No merging in ifelse * Remove scope builder * Add ops * Replace < with less * Linter * Pass Tests * Change back to static const * Cpplinter * address PR comments' * PR Comments * Clang-format check * PR Comments * PR Comments * Change back to Insert Ann in AnnotateARgs Co-authored-by: Ritwik Das <dasritwi@3c22fb14d7c6.ant.amazon.com> Co-authored-by: Ubuntu <ubuntu@ip-172-31-3-223.us-west-2.compute.internal>
* Change annotate target * Annotate_target * Revert namespace changes * Add tests for if-else node * Add while_let testcase * No merging in ifelse * Remove scope builder * Add ops * Replace < with less * Linter * Pass Tests * Change back to static const * Cpplinter * address PR comments' * PR Comments * Clang-format check * PR Comments * PR Comments * Change back to Insert Ann in AnnotateARgs Co-authored-by: Ritwik Das <dasritwi@3c22fb14d7c6.ant.amazon.com> Co-authored-by: Ubuntu <ubuntu@ip-172-31-3-223.us-west-2.compute.internal>
7 participants
This PR adds changes to annotate_target to handle control flow(if-else nodes) and let nodes correctly.
Context: The Neo team was trying to compile TF MobileNet SSD model and since the model is dynamic we noticed that the if-else and let nodes are not getting correctly compiled. Therefore this PR addresses those fixes.