[Relay] IndexedGraph improvements in preparation for Collage#11481
Merged
jwfromm merged 5 commits intoapache:mainfrom Jun 7, 2022
Merged
[Relay] IndexedGraph improvements in preparation for Collage#11481jwfromm merged 5 commits intoapache:mainfrom
jwfromm merged 5 commits intoapache:mainfrom
Conversation
Contributor
|
Are the properties in |
Contributor
Author
|
I'd forgotten I'd adjusted the visit order in IndexedGraph to respect idx(x) < idx(y) => y does not depend on x with let-bound vars. That deserves proper unit testing. Stand by. |
mbs-octoml
changed the title
mbs-octoml
force-pushed
the
mbs-collage-patterns
branch
2 times, most recently
from
466acce to
3031c73
Compare
mbs-octoml
changed the title
mbs-octoml
force-pushed
the
mbs-collage-patterns
branch
from
3031c73 to
ab76f30
Compare
mbs-octoml
changed the title
Contributor
Author
|
This is now ready for review. |
mbaret
reviewed
Jun 6, 2022
Contributor
mbaret
left a comment
mbaret
left a comment
There was a problem hiding this comment.
Mostly just some comments on the PR structure. The changes to IndexedGraph look superficially good to me, but I'm not that familiar with the code there. It'd be great for @mbrookhart to take a look over those elements specifically.
mbs-octoml
force-pushed
the
mbs-collage-patterns
branch
from
ab76f30 to
64ed6e9
Compare
Contributor
Author
|
Thanks @mbaret , PTAL. Unfortunately Matthew is out for a while so it's up to us to build up expertise ourselves. |
- Complete the implementation of WithFields. (Unfortunately they appear to be without unit tests and I continue this tradition...) - InferTypeExpr for InferTypeLocal but return the expression rather than the type. - Remove python binding of InlineComposites since C++ impl was removed some time ago. - Make IndexedGraph<Expr/DFPattern> more robust as stand-alone datastructure, and avoid unnecessary copies. This will become a fundamental datastructure in Collage rather than just a helper for DFPatternMatcher. - Extend IndexedGraph with a notion of 'basic block' on every dataflow node. Needed by Collage to avoid impossible partitions.
- More tests
- More tests
mbs-octoml
force-pushed
the
mbs-collage-patterns
branch
from
7b37d9a to
bd7200c
Compare
jwfromm
approved these changes
Jun 7, 2022
Contributor
jwfromm
left a comment
jwfromm
left a comment
There was a problem hiding this comment.
Thanks for the work @mbs-octoml and the review @mbaret. This LGTM so I'm going to go ahead and merge it.
mbs-octoml
added a commit
to mbs-octoml/mbs-tvm
that referenced
this pull request
Jun 7, 2022
I goofed in apache#11481 in moving a dominates check to an ICHECK (and I'm confused how it got through ci?). It is ok to match a sub-graph which has dataflow outside of the sub-graph, provided all such flows eventually come into the sub-graph.
Contributor
Author
|
Shoot I goofed and will need #11616 to fix it. |
Kathryn-cat
pushed a commit
to Kathryn-cat/tvm
that referenced
this pull request
Jun 10, 2022
…11481) * [Relay] Odd's 'n ends changes to help Collage. - Complete the implementation of WithFields. (Unfortunately they appear to be without unit tests and I continue this tradition...) - InferTypeExpr for InferTypeLocal but return the expression rather than the type. - Remove python binding of InlineComposites since C++ impl was removed some time ago. - Make IndexedGraph<Expr/DFPattern> more robust as stand-alone datastructure, and avoid unnecessary copies. This will become a fundamental datastructure in Collage rather than just a helper for DFPatternMatcher. - Extend IndexedGraph with a notion of 'basic block' on every dataflow node. Needed by Collage to avoid impossible partitions. * - Revert non IndexedGraph changes. * - Stick to 'Indexed graph' terminology - More tests * - Stick to 'Indexed graph' terminology - More tests * - Remove silly unit test
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
IndexedGraph turns out to be a fundamental datastructure in Collage. This PR gives it some polish.