[Unity] Add instruments to relay translator #15601
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sometimes its useful to instrument relay passes that are run during relay to relax translation, and this patch adds a new argument to relay translator to accept an instruments list argument that gets passed onto the PassContext used while running relay prefix passes
|
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 |
|
@tvm-bot rerun |
|
Failed to re-run CI in https://github.com/apache/tvm/actions/runs/5934313794 with response |
|
@tvm-bot rerun |
|
Failed to re-run CI in https://github.com/apache/tvm/actions/runs/5934428367 with response |
|
@tvm-bot rerun |
|
Failed to re-run CI in https://github.com/apache/tvm/actions/runs/5935375081 with response |
quic-sanirudh
force-pushed
the
add_instruments_support_relay_translator
branch
from
21af3e2 to
685cc76
Compare
Lunderberg
requested changes
Aug 27, 2023
| @@ -255,6 +261,7 @@ def visit_func(node): | |||
| opt_level=opt_level, | |||
| config=pass_config, | |||
| disabled_pass=disabled_pass, | |||
| instruments=instruments, | |||
There was a problem hiding this comment.
I like it, though I'm wondering if we should have it forward the instrumentation of the existing PassContext, rather than explicitly accept it as an argument. That way, debug instrumentation set by a much higher scope would also be preserved, without requiring an update to the immediately-calling scope.
instruments = tvm.transform.PassCurrent().instrumentsThere was a problem hiding this comment.
Thanks for your review @Lunderberg.
I actually did it that way initially, but since the other arguments to PassContext such as opt_level and pass_config were taken as arguments to the relay translator, I modified it take instruments as an argument as well.
I can change it back to be forwarded from current PassContext as you've suggested, either way is fine with me, but in that case, should we also remove opt_level and pass_config from being arguments and forward them from current context as well? Since that would be a breaking change, I'm a bit hesitant to do that.
There was a problem hiding this comment.
Hmm, good point. Since the other arguments are currently specified explicitly, probably better to keep it that way for consistency.
In the long-term, I think that entering a PassContext should probably distinguish between options that override the previous (opt_level), options that append to the previous (instruments, disabled_passes).
Lunderberg
approved these changes
Aug 27, 2023
Sure, I agree that in the long term, it might be better to pick it up from PassContext. Perhaps to avoid a breaking change initially, we can keep the arguments and prioritize them when provided, but when they're not provided, they can be picked up from the current PassContext. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Sometimes it is useful to instrument relay passes that are run during relay to relax translation, and this patch adds a new argument to relay translator to accept instruments list.
The instruments can be passed on as