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

Allow for folding of ElemKind conversion into IO where applicable for better bounds checking/testing #3807

Closed
wants to merge 3 commits into from

Conversation

@jfix71
Copy link
Contributor

jfix71 commented Nov 22, 2019

Summary: This PR introduces a FoldElemKindConversionIntoIO pass which looks for single-use Placeholders that have ConvertTo/Quantize/Dequantize right after/before them and folds them into the Placeholder (and SaveNode if an output). It then adds usage of this pass to compareAgainstInterpreter() tests. By doing this we allow the backend to more directly test the operator in its desired precision instead of also allowing the results of the test to be impacted by the specific logic used for type conversion (see bottom for before/after Functions).

This is opt-in, and at least initially meant for better bounds testing on tests which use compareAgainstInterpreter(). This is a dangerous pass, as it requires converting the associated Tensors for these Placeholders in PlaceholderBindings, and correctly getting handles to these Tensors based on these types. I've made this opt-in based on the optimization options in the compilation context.

Test Plan: All tests still pass. I modified the one test (BackendCorrectnessTest's basicFCNetQuantized) which currently does Int8 to Int8 comparison to expect bitwise accuracy.

Related to #3710

Before:
before

After:
after

Copy link

facebook-github-bot left a comment

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

@jfix71 jfix71 force-pushed the jfix71:compare_interp_no_conversion branch from 6967ac6 to cfbcf7f Nov 25, 2019
Copy link

facebook-github-bot left a comment

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

/// Helper that iterates over all of the Placeholders in \p PHs and converts the
/// Tensor pair found in \p bindings to the same type as the Placeholder if
/// necessary.
static void convertBindingsToCorrectType(PlaceholderBindings &bindings,

This comment has been minimized.

Copy link
@gcatron

gcatron Dec 3, 2019

Contributor

Does a seperate PH list need to be passed in here? We could iterate over all PHs in the bindings.

This comment has been minimized.

Copy link
@jfix71

jfix71 Dec 3, 2019

Author Contributor

@gcatron Hm so after reading it again, I think I did it like this because I wanted to be positive I only changed the placeholders that came from the specific Function I was compiling (so I pass in Function::findPlaceholders() to this function). I can't think of a specific case where we wouldn't want to fix any other mismatches but I guess since it felt like an aggressive thing to do to convert Tensors I was being conservative. I think I'm fine with changing it to convert all of the bindings but no strong feelings, WDYT?

This comment has been minimized.

Copy link
@gcatron

gcatron Dec 3, 2019

Contributor

I think it's fine as is then. Being more specific also allows for finer grained testing too.

@gcatron
gcatron approved these changes Dec 3, 2019
Copy link
Contributor

gcatron left a comment

One nit, otherwise looks good!

@jfix71 jfix71 force-pushed the jfix71:compare_interp_no_conversion branch from cfbcf7f to 700a149 Dec 3, 2019
Copy link

facebook-github-bot left a comment

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

@jfix71 jfix71 deleted the jfix71:compare_interp_no_conversion branch Dec 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.