Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Relay] IndexedGraph improvements in preparation for Collage #11481

Merged
merged 5 commits into from
Jun 7, 2022
Merged

[Relay] IndexedGraph improvements in preparation for Collage #11481

merged 5 commits into from
Jun 7, 2022

Conversation

mbs-octoml
Copy link
Contributor

@mbs-octoml mbs-octoml commented May 26, 2022 •
edited
Loading

IndexedGraph turns out to be a fundamental datastructure in Collage. This PR gives it some polish.

  • Avoid unnecessary copies.
  • Hide implementation so access violations are easy to spot.
  • Add notion of basic block to IndexedGraph::Node so Collage can avoid unioning sub-graphs which cross control flow boundaries.
  • Make sure dataflow graph is always a DAG, even with let-rec bound functions.
  • Fix dataflow order for let-binding to be from let-bound value to let-bound var to all uses of let-bound var.
  • Make sure all the Relay constructs are accounted for.
  • Add C++ unit test (since not intended to be an FFI-friendly datastructure).

@slyubomirsky
Copy link
Contributor

Are the properties in WithFields documented somewhere? That probably shouldn't be too difficult to unit-test.

@mbs-octoml
Copy link
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 mbs-octoml marked this pull request as draft June 1, 2022 16:11
@mbs-octoml mbs-octoml changed the title [Relay] Odd's 'n ends changes to help Collage. [Relay] DRAFT: Odd's 'n ends changes to help Collage. Jun 1, 2022
@mbs-octoml mbs-octoml force-pushed the mbs-collage-patterns branch 2 times, most recently from 466acce to 3031c73 Compare June 2, 2022 23:30
@mbs-octoml mbs-octoml changed the title [Relay] DRAFT: Odd's 'n ends changes to help Collage. [Relay] DRAFT: IndexedGraph improvements for Collage Jun 2, 2022
@mbs-octoml mbs-octoml force-pushed the mbs-collage-patterns branch from 3031c73 to ab76f30 Compare June 3, 2022 15:57
@mbs-octoml mbs-octoml marked this pull request as ready for review June 3, 2022 15:57
@mbs-octoml mbs-octoml changed the title [Relay] DRAFT: IndexedGraph improvements for Collage [Relay] IndexedGraph improvements in preparation for Collage Jun 3, 2022
@mbs-octoml
Copy link
Contributor Author

This is now ready for review.

mbaret
mbaret reviewed Jun 6, 2022
View reviewed changes
Copy link
Contributor

@mbaret mbaret left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

include/tvm/relay/expr_functor.h Outdated Show resolved Hide resolved
include/tvm/relay/transform.h Outdated Show resolved Hide resolved
python/tvm/relay/transform/transform.py Outdated Show resolved Hide resolved
src/runtime/contrib/tensorrt/tensorrt_ops.cc Outdated Show resolved Hide resolved
tests/python/contrib/test_ethosu/test_codegen.py Outdated Show resolved Hide resolved
src/relay/ir/dataflow_matcher_impl.h Outdated Show resolved Hide resolved
src/relay/ir/expr.cc Outdated Show resolved Hide resolved
tests/python/relay/test_dataflow_pattern.py Show resolved Hide resolved
src/relay/op/dyn/tensor/transform.cc Show resolved Hide resolved
src/relay/ir/indexed_graph.h Outdated Show resolved Hide resolved
@mbs-octoml mbs-octoml force-pushed the mbs-collage-patterns branch from ab76f30 to 64ed6e9 Compare June 6, 2022 20:40
@mbs-octoml
Copy link
Contributor Author

Thanks @mbaret , PTAL.

Unfortunately Matthew is out for a while so it's up to us to build up expertise ourselves.

mbs-octoml added 5 commits June 7, 2022 09:11
@mbs-octoml
[Relay] Odd's 'n ends changes to help Collage.
3797ab6 
 - 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.
@mbs-octoml
- Revert non IndexedGraph changes.
b2bf710
@mbs-octoml
- Stick to 'Indexed graph' terminology
cfb0ce6 
- More tests
@mbs-octoml
- Stick to 'Indexed graph' terminology
530f25b 
- More tests
@mbs-octoml
- Remove silly unit test
bd7200c
@mbs-octoml mbs-octoml force-pushed the mbs-collage-patterns branch from 7b37d9a to bd7200c Compare June 7, 2022 16:11
jwfromm
jwfromm approved these changes Jun 7, 2022
View reviewed changes
Copy link
Contributor

@jwfromm jwfromm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the work @mbs-octoml and the review @mbaret. This LGTM so I'm going to go ahead and merge it.

@jwfromm jwfromm merged commit d8f57ed into apache:main Jun 7, 2022
@mbs-octoml mbs-octoml deleted the mbs-collage-patterns branch June 7, 2022 19:35
mbs-octoml added a commit to mbs-octoml/mbs-tvm that referenced this pull request Jun 7, 2022
@mbs-octoml
[Relay] Restore dominator check
e49270b 
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.
@mbs-octoml mbs-octoml mentioned this pull request Jun 7, 2022
Merged
@mbs-octoml
Copy link
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
@mbs-octoml @Kathryn-cat
[Relay] IndexedGraph improvements in preparation for Collage (apache#…
b7bbe5e 
…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
@comaniac comaniac mentioned this pull request Jun 13, 2022
Closed
@hgt312 hgt312 mentioned this pull request Jun 14, 2022
Merged
@driazati driazati mentioned this pull request Jul 14, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mbaret mbaret mbaret left review comments

@jwfromm jwfromm jwfromm approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mbs-octoml @slyubomirsky @jwfromm @mbaret