Skip to content

[GraphOptimizer] Fix bug in constant folding optimization. #3500

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
Closed
shajrawi opened this issue Sep 11, 2019 · 1 comment
Closed

[GraphOptimizer] Fix bug in constant folding optimization. #3500

shajrawi opened this issue Sep 11, 2019 · 1 comment

Comments

@shajrawi
Copy link
Contributor

We're creating an Interpreter backend, but we are also running constant folding after lowering. This can be bad on backends that create transformations that break Glow's canonical tensor layout.
See NonSquarePaddingConvolution on the OpenCL backend for example. We have a Reshape node in NCHW layout. Constant folding creates the following function for it:

Graph structure __constEvaluationFunction__:
Reshape
name : conv_filter1
Input : float<2 x 2 x 2 x 1>
Dims : [2, 1, 2, 2]
Layout : NCHW
users : 1
Result : float<2 x 1 x 2 x 2>
Save
name : conv_filter11
Input : float<2 x 1 x 2 x 2>
Output : float<2 x 1 x 2 x 2>
users : 0

If we run a canonical layout verifier on said function, see #3452, it is gonna blow up: Save is getting NCHW layout but expects NHWC layout. The constant function that we created is bad.
@shajrawi
Copy link
Contributor Author

CC @opti-mix

@shajrawi shajrawi mentioned this issue Sep 11, 2019
Closed
shajrawi added a commit to shajrawi/glow that referenced this issue Nov 6, 2019
@shajrawi
Teach Glow how to support layout requirements and fix uncovered bugs.
b1fac3f 
Fixes pytorch#3452

Also Fixes pytorch#3493 and pytorch#3500 GraphOptimizer bugs which were found after adding the layout verifier.

Provides a workaround for the pytorch#3499 issue which was also found via the verifier.

Note: I did not want to break the `enum ConvolutionLayout` introduced in 5074a72, As such, I used it in the verifier / did not change the creation of said nodes.
HOWEVER: We should use the more-generic string-based layout, which I introduce to Transpose node in this commit: it is basically an extendable enum that can be used in the backends without touching the generic code base. as a bonus, it makes differentiation easier: see how it is done for transpose now in `Function *glow::differentiate`.

Getting rid of said enum is a proposed TODO / follow-up.

Also note that some nodes *need* layout requirements, which have been added, namely we need to know the layout for placeholders and constants (obviously) and for reshapes (in case we optimized a transpose into a reshape.

An additional nice-to-have feature of the string-based layout is the wildcard / any-layout option. Some operations, such as data parallel nodes, might accept any layout.

A potential follow-up is to get create a "Solver" that automatically inserts transposes if the layouts do not match, this might greatly simplify the loader: we no longer need to insert transposes based on if we are importing NHWC or NCHW (for example). We just need to annotate the placeholder with the layout information we've get at load-time, and which we "forget" afterwards.

The verifier is useful even without creating said solver, it exposed a couple of bugs which are mentioned in this commit, as such any proposed solvers are not a must-have to demonstrate the usefulness of this commit.
vdantu pushed a commit to vdantu/glow that referenced this issue Jul 12, 2020
@shajrawi @vdantu
Teach Glow how to support layout requirements and fix uncovered bugs. (…
9ac362c 
…pytorch#3503)

Summary:
Note: I did not want to break the `enum ConvolutionLayout` introduced in 5074a72, As such, I used it in the verifier / did not change the creation of said nodes.
HOWEVER: We should use the more-generic string-based layout, which I introduce to Transpose node in this commit: it is basically an extendable enum that can be used in the backends without touching the generic code base. as a bonus, it makes differentiation easier: see how it is done for transpose now in `Function *glow::differentiate`.

Getting rid of said enum is a proposed TODO / follow-up.

Also note that some nodes *need* layout requirements, which have been added, namely we need to know the layout for placeholders and constants (obviously) and for reshapes (in case we optimized a transpose into a reshape).

An additional nice-to-have feature of the string-based layout is the wildcard / any-layout option. Some operations, such as data parallel nodes, might accept any layout.

A potential follow-up is to get create a "Solver" that automatically inserts transposes if the layouts do not match, this might greatly simplify the loader: we no longer need to insert transposes based on if we are importing NHWC or NCHW (for example). We just need to annotate the placeholder with the layout information we've get at load-time, and which we "forget" afterwards.

The verifier is useful even without creating said solver, it exposed a couple of bugs which are mentioned in this commit, as such any proposed solvers are not a must-have to demonstrate the usefulness of this commit.

Fixes pytorch#3452

Also Fixes pytorch#3493 and Fixes pytorch#3500 GraphOptimizer bugs which were found after adding the layout verifier.

Provides a workaround for the pytorch#3499 issue which was also found via the verifier.
Pull Request resolved: pytorch#3503

Test Plan: `ninja test`

Differential Revision: D18357369

Pulled By: shajrawi

fbshipit-source-id: 45f91fbe120b234c2a85879cee9ee0de6c100b50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Teach Glow how to support layout requirements and fix uncovered bugs. shajrawi/glow
1 participant
@shajrawi