Skip to content

[GraphOptimizer] Reshape sinking pass #5616

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

Closed

Conversation

aturetsk
Copy link
Contributor

Summary:

Add Reshape sinking pass in GraphOptimizer. Currently supports only sinking below unary eltwise operators.
The sinking logic is implemented using generic node interfaces and properties (DataParallel), as opposed to having dedicated node-specific code for each operator. The benefits of this approach are:

  1. Every unary elementwise operator is covered by the pass. If we ever add a new unary eltwise operator it's going to participate in Reshape sinking transformations automatically.
  2. Backend-specific nodes (if they are unary and marked as DataParallel in node description) will benefit from this transformation as well, i.e. no need for backend to implement it.

Also, remove Clip sinking below Reshape, as Clip is handled by Reshape sinking pass now.

Documentation:
N/A

Fixes #5247 (partially)

Test Plan:
Added a GraphOptz unit test.

@aturetsk aturetsk force-pushed the cadence/turetski/reshape-sinking branch from 5fd96af to 1f936f7 Compare May 4, 2021 22:45
@aturetsk
Copy link
Contributor Author

cc: @jfix71

Copy link
Contributor

@jfix71 jfix71 left a comment

Choose a reason for hiding this comment

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

🚢

@facebook-github-bot
Copy link

@jfix71 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@aturetsk
Copy link
Contributor Author

@jfix71, thanks for review.

Would that be ok if I refactor Transpose sinking transformations for unary/binary element-wise operators in the same way (to make them use generic Node interfaces and DataParallel property) to get the same benefits I listed above? IMO having a set of generic optimizations which are enabled for a node if it has a specific property is a good thing. What's your opinion?

@jfix71
Copy link
Contributor

jfix71 commented May 12, 2021

@aturetsk Yeah, I think it's generally a good idea. We have this data parallel marker but aren't using it very well 🙂 We had issue #1830 a while back which mentions something similar.

@aturetsk
Copy link
Contributor Author

@jfix71, can we land this?

@facebook-github-bot
Copy link

@jfix71 merged this pull request in 4139e75.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[GraphOptimizer] Reshape sinking
3 participants