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

[GraphOptimizer] graph optimizer doesn't optimize away non-inverses consecutive Tranposes #2013

Closed
jackm321 opened this issue Nov 13, 2018 · 9 comments

Comments

@jackm321
Copy link
Contributor

Two tranposes next to each other in the graph could be collapsed into a single tranpose but this isn't done by the GraphOptimizer. I haven't seen this in any network and maybe it's not a situation that is likely to arise from any real network.

@SplitInfinity
Copy link
Contributor

I doubt someone would intentionally write a network like this, but could this sequence appear as a by-product of existing optimisations?

@jackm321
Copy link
Contributor Author

jackm321 commented Dec 4, 2018

@SplitInfinity one of the things that GraphOptimizer does is try to sink transposes below other nodes so that those transposes are more likely end up next to each other in the graph so that they can be eliminated. So two transposes that may not have initially been next to one another often end up next to each other after some graph optimization passes.

@Squadrick
Copy link
Contributor

@jackm321 Can I work on this?

@rdzhabarov
Copy link
Contributor

We'd love contributions @Squadrick

@Squadrick
Copy link
Contributor

@rdzhabarov I've read the CONTRIBUTING.md, and about the template for PRs. Anything else I need to know before I start off?

Also, could one of you give me an elaborate explanation of the optimization? In docs/Optimizations.md it's mentioned:

Elimination of transpose operations reversing each other

How is what's mentioned here any different?

@jfix71
Copy link
Contributor

jfix71 commented Jan 17, 2019

How is what's mentioned here any different?

@Squadrick Right now we eliminate two consecutive transposes when they are the inverse of each other -- that is, we eliminate both of them, because when combined they are a noop. Here's the check for this:

/// \returns true if the masks \p shuffle1 and shuffle2 are
/// the inverse of on another. Applying both masks should result in the identity
/// shuffle.
static bool isIdentityShuffle(llvm::ArrayRef<unsigned_t> shuffle1,
llvm::ArrayRef<unsigned_t> shuffle2) {

The new optimization would be to merge two consecutive transposes that aren't the inverse of each other into a single transpose. E.g.:

y = transpose(x, {1, 0 ,2}) // x is 3 dimensional; this swaps the first two dimensions
z = transpose(y, {0, 2, 1}) // this swaps the last two dimensions

This could be done with a single transpose:

z = transpose(x, {1, 2, 0})

@rdzhabarov
Copy link
Contributor

I've read the CONTRIBUTING.md, and about the template for PRs. Anything else I need to know before I start off?

Yup, that's good. Also look https://github.com/pytorch/glow/blob/master/docs/CodingStandards.md.

@Squadrick
Copy link
Contributor

Got it, I'll make a PR in a couple of hours, just started working on it.

@Squadrick
Copy link
Contributor

Sorry about the delay. Submitted the PR.

jfix71 pushed a commit that referenced this issue Feb 4, 2019
### Description
* Before: GraphOptimizer would only combine consecutive tranposes if the resultant was an identity transpose.
* After: GraphOptimizer combines all consecutive tranposes, including non-inverse pairs.

### Testing
* `mergeNonInverseTransposes` replaces `dontCancelTwoTransposesIfNotMatching` unit test in `GraphOptzTest.cpp`. 
* The unit test checks that multiple consecutive non-inverse transposes are combined into a single transpose node.

### Documentation
* Documentation has been updated in `docs/Optimizations.md` under graph optimization.

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

No branches or pull requests

6 participants