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.

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

[NNAPI QDQ] AddQDQAdd/Mul, update to NNAPI QDQ handling, update some test settings #10483

Merged
merged 7 commits into from
Feb 9, 2022
Merged

[NNAPI QDQ] AddQDQAdd/Mul, update to NNAPI QDQ handling, update some test settings #10483

merged 7 commits into from
Feb 9, 2022

Conversation

guoyu-wang
Copy link
Contributor

Description: [NNAPI QDQ] AddQDQAdd/Mul, update to NNAPI QDQ handling, update some test settings

Motivation and Context

  • Add QDQ Add/Mul for NNAPI
  • Update the NNAPI input/output checking, consolidated some helper functions
  • Update the test cases to generate NNAPI specific QDQ models
  • Update the NNAPI test to have optional verification for entire graph taken by EP
  • Added options for RunAndVerifyOutputsWithEP to verify fp32 results with optional tolerance
  • Fixed a small issue in previous QDQ AvergeagePool PR

@guoyu-wang
Squashed commit of the following:
ede386c 
commit 1238049
Author: Guoyu Wang <[email protected]>
Date:   Mon Feb 7 12:59:04 2022 -0800

    Add qdq mul support

commit 9cadda7
Merge: 7a32847 0f5d0a0
Author: Guoyu Wang <[email protected]>
Date:   Mon Feb 7 11:24:47 2022 -0800

    Merge remote-tracking branch 'origin/master' into gwang-msft/qdq_mul

commit 7a32847
Author: Guoyu Wang <[email protected]>
Date:   Mon Feb 7 00:41:30 2022 -0800

    move test case to util

commit c1a8f0d
Author: Guoyu Wang <[email protected]>
Date:   Fri Feb 4 13:04:26 2022 -0800

    update input/output check

commit a6f0a0d
Author: Guoyu Wang <[email protected]>
Date:   Thu Feb 3 18:37:21 2022 -0800

    update quantized io check functions

commit 87f4d1d
Merge: 7849f07 97b8f6f
Author: Guoyu Wang <[email protected]>
Date:   Wed Feb 2 17:22:58 2022 -0800

    Merge remote-tracking branch 'origin/master' into gwang-msft/qdq_mul

commit 7849f07
Author: Guoyu Wang <[email protected]>
Date:   Wed Feb 2 17:22:55 2022 -0800

    minor update

commit 7196cdf
Author: Guoyu Wang <[email protected]>
Date:   Wed Feb 2 10:50:10 2022 -0800

    init change

commit 84c0077
Merge: a8c7dce 7318361
Author: Guoyu Wang <[email protected]>
Date:   Tue Feb 1 18:21:17 2022 -0800

    Merge remote-tracking branch 'origin/master' into gwang-msft/qdq_mul

commit a8c7dce
Merge: 55e536c ef7b4dc
Author: Guoyu Wang <[email protected]>
Date:   Tue Feb 1 13:51:04 2022 -0800

    Merge remote-tracking branch 'origin/master' into gwang-msft/qdq_mul

commit 55e536c
Author: Guoyu Wang <[email protected]>
Date:   Tue Feb 1 11:44:34 2022 -0800

    address cr comments

commit d460f5b
Author: Guoyu Wang <[email protected]>
Date:   Tue Feb 1 00:33:54 2022 -0800

    fix android UT failure

commit 52146cf
Author: Guoyu Wang <[email protected]>
Date:   Mon Jan 31 16:01:13 2022 -0800

    fix build break

commit ec6d07d
Author: Guoyu Wang <[email protected]>
Date:   Mon Jan 31 15:41:52 2022 -0800

    minor update to UT

commit 8ec8490
Author: Guoyu Wang <[email protected]>
Date:   Mon Jan 31 15:01:30 2022 -0800

    Add NNAPI support of QDQ Resize
skottmckay
skottmckay reviewed Feb 8, 2022
View reviewed changes
onnxruntime/test/optimizer/qdq_test_utils.h
auto* output_arg = builder.MakeOutput();

#ifdef USE_NNAPI // NNAPI require consistent scales for DQ -> bin op -> Q
float q_scale = 0.008f;
Copy link
Contributor

@skottmckay skottmckay Feb 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Is this true for all binary ops? Naively I would have expected mul or div to potentially change the scale but maybe that isn't the case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Here's a snippet from a QDQ model exported from pytorch where the scale and zero point changes across an Add node.

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the output scale and ZP can be changed, updated the test case, it is only required to be the same for QD->bin_op_input and bin_op_output->Q

edgchen1
edgchen1 reviewed Feb 8, 2022
View reviewed changes
onnxruntime/core/providers/nnapi/nnapi_builtin/builders/op_support_checker.cc Outdated
op_registrations.support_checkers.push_back(std::make_unique<T>());
for (const auto& op : op_types) {
op_registrations.op_support_checker_map.emplace(op, op_registrations.support_checkers.back().get());
const auto& scale_tensor = *initializers.at(scale_name);
Copy link
Contributor

Choose a reason for hiding this comment

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

at

general comment: double lookup of Contains(container, key) then container.at(key) can be changed to auto it = container.find(key); it != container.end() and it->second

Copy link
Contributor

Choose a reason for hiding this comment

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

also shows up in HasExternalInitializer(), IsQuantizationZeroPointSupported(), ReshapeOpSupportChecker::IsOpSupportedImpl(), and ConvOpSupportChecker::IsOpSupportedImpl()
can also address it later

@YUNQIUGUO YUNQIUGUO mentioned this pull request Feb 8, 2022
Merged
edgchen1
edgchen1 approved these changes Feb 9, 2022
View reviewed changes
Copy link
Contributor

@edgchen1 edgchen1 left a comment

Choose a reason for hiding this comment

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

:shipit:

@guoyu-wang guoyu-wang merged commit e4dc4e4 into master Feb 9, 2022
@guoyu-wang guoyu-wang deleted the gwang-msft/nnapi_qdq_mul branch February 9, 2022 04:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@YUNQIUGUO YUNQIUGUO YUNQIUGUO left review comments

@skottmckay skottmckay skottmckay left review comments

@edgchen1 edgchen1 edgchen1 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@guoyu-wang @skottmckay @edgchen1 @YUNQIUGUO