-
Notifications
You must be signed in to change notification settings - Fork 3.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add LowerTEPass, and convert calls to LowerTE to application of LowerTEPass #8802
Merged
jroesch
merged 12 commits into
apache:main
from
electriclilies:remove-target-map-hacky-version
Aug 24, 2021
Merged
Changes from all commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
de2ad60
Initial commit
electriclilies 15ca3f3
Fix bad rebase
electriclilies 5a97dd0
Address 1st round of comments
electriclilies 818bf0f
Use tir kTarget instead of relay one
electriclilies ad0059b
Change target string to Target obj
electriclilies 6be39dd
removing target string causing issues
electriclilies d698857
Fix typos
electriclilies 120b14a
Revert target str -> target obj changes
electriclilies 9015163
Don't use Update : IRModule because it is broken
electriclilies ce57b2e
Fix check
electriclilies 8521ce2
flaky test?
electriclilies 772ae10
lint
electriclilies File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[A question to the community and not specific to your PR Lily!]
This is a good example of code which could be easily unit tested in C++ in the, er, 'conventional' sense. That is, as a reader I could expect to go to tests/cpp/relay/backend/te_compiler_test.cc and look for TEST(IRModuleToLoweredModule, ...). Currently this new code is tested indirectly via it's use by LowerTEPass and consumers of such, which in turn are tested indirectly by virtue of everything passing into TIR via this choke point. Just wanted to test the water on whether folks on this PR have opinions here so I don't go off tilting at windmills.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good point. Since these two functions are supposed to be inverses of each other, it would be pretty easy to write a unit test for it in theory. When I was developing, I actually inserted the conversions in some places and ran existing unit tests to make sure that the functions worked, but it would be great to have a way to directly write unit tests in C++. That way I wouldn't have to remove my tests before merging!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently when we unit-test passes by exposing them via a python API, construct the expected input and output and run the tests in python:
https://github.com/apache/tvm/blob/main/tests/python/unittest/test_tir_transform_loop_partition.py#L30
There are certainly pros and cons of doing so. The original rationale is that we require most of the compiler passed to be accessible from python and it is relatively easier to construct and expand test cases.
We could revisit this pt on the need of the related testcases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@electriclilies is there any reason you can't create a file similar to https://github.com/apache/tvm/blob/main/tests/cpp/build_module_test.cc and test the functions there?
Ideally I'd definitely like to see a C++ test setup as @mbs-octoml describes rather than the single folder but this would work here? It's not an absolute rule that we must expose via Python for testing is it @tqchen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for most of the passes that can be modularized, we encourage the python first principle and expose via python. This one is a intermediate state so it is not an absolute rule to do so
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the comments. Perhaps a rule of thumb here is if it's part of the public api it should be tested on the py side, but otherwise should stay on the c++ side. I'm struggling to see how to write targeted unit tests on the py side without both risking making something internal part of the defacto api and without paying for all the unit test boundaries be ffi-able.