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.

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

[BYOC] Configurable optimize pass for PartitionGraph #6777

Closed
wants to merge 2 commits into from

Conversation

comaniac
Copy link
Contributor

Currently, PartitionGraph implicitly invokes relay.ext.*.optimize pass for each partitioned Relay function, but this is hard to maintain and trace. This PR adds an optional argument to PartitionGraph pass to make it explicit.

cc @zhiics @masahi @lhutton1

Copy link
Contributor

@lhutton1 lhutton1 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@manupak manupak left a comment

Choose a reason for hiding this comment

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

A minor high-level nit regarding the placement of optimize pass.

@@ -62,7 +72,7 @@ def partition_for_arm_compute_lib(mod, params=None):
transform.InferType(),
transform.MergeComposite(arm_compute_lib_pattern_table()),
transform.AnnotateTarget("arm_compute_lib"),
transform.PartitionGraph(),
transform.PartitionGraph(optimize),
Copy link
Contributor

Choose a reason for hiding this comment

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

Why cant we just run the optimize pass before partition graph here ? Is there a reason why it has to be inside PartitionGraph() ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This pass applies to each partitioned function, so it has to be called after the partitioned function has been created. See #6068

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, then why not after PartitionGraph ? (using kCompiler as a filter) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As being said in that PR, we can definitely do that. In fact we are planning an RFC right now, so depending on one the RFC, we probably don't need this PR anymore.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, sounds good :). I think PartitionGraph should just perform partitioning unless its really unavoidable to perform the needed post-processing after the pass.

@tqchen
Copy link
Member

tqchen commented Dec 9, 2020

@comaniac consider close this PR as it seems to be stale?

@comaniac
Copy link
Contributor Author

comaniac commented Dec 9, 2020

@comaniac consider close this PR as it seems to be stale?

Ah sorry we don't have bandwidth to work on this recently. I'll close the PR for now.

@comaniac comaniac closed this Dec 9, 2020
@comaniac comaniac deleted the explicit_partition_opt branch March 12, 2021 22:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants