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

Quantize slice op #37630

Merged
merged 3 commits into from
Dec 7, 2021
Merged

Quantize slice op #37630

merged 3 commits into from
Dec 7, 2021

Conversation

zuzg
Copy link
Contributor

@zuzg zuzg commented Nov 26, 2021 •
edited
Loading

PR types

New features

PR changes

OPs

Describe

Quantization of slice operator.

In Ernie model (test_quant2_int8_ernie_mkldnn) quantized 1 sliced op, what caused drop in latency by 0.595%.

@paddle-bot-old
Copy link

Thanks for your contribution!
Please wait for the result of CI firstly. See Paddle CI Manual for details.

wozna
wozna previously approved these changes Nov 29, 2021
View reviewed changes
Copy link
Contributor

@wozna wozna left a comment

Choose a reason for hiding this comment

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

LGTM

@zuzg
Copy link
Contributor Author

zuzg commented Nov 29, 2021

@lidanqing-intel @sfraczek please review

lidanqing-intel
lidanqing-intel previously approved these changes Nov 29, 2021
View reviewed changes
Copy link
Contributor

@lidanqing-intel lidanqing-intel left a comment

Choose a reason for hiding this comment

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

LGTM

@lidanqing-intel
Copy link
Contributor

lidanqing-intel commented Nov 29, 2021 •
edited
Loading

Hi, @baoachun Do you think this PR could be approved? This PR added new attributes with asExtra. Slice op is op without weights and its quantization is easier.

XieYunshen
XieYunshen previously approved these changes Nov 30, 2021
View reviewed changes
Copy link
Contributor

@XieYunshen XieYunshen left a comment

Choose a reason for hiding this comment

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

LGTM for set_tests_properties(test_analyzer_ernie_int8 PROPERTIES TIMEOUT 120)

sfraczek
sfraczek reviewed Nov 30, 2021
View reviewed changes
paddle/fluid/framework/ir/mkldnn/cpu_quantize_pass_tester.cc Outdated
@@ -784,6 +788,113 @@ TEST(CpuQuantizePass, reshapeBetweenNonQuantizedOp) {
added_nodes_count, 2.0f * 127);
}

static const std::initializer_list<std::string> variable_names_slice = {
"a", "w1", "b", "c", "d", "e", "f"};
Copy link
Contributor

Choose a reason for hiding this comment

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

it seems you are not using 3 variables: w1,e,f

paddle/fluid/operators/slice_op.cc Outdated
@@ -244,7 +244,7 @@ class SliceOpMaker : public framework::OpProtoAndCheckerMaker {
"mkldnn_data_type",
"(string, default \"float32\"). Data type of mkldnn kernel")
.SetDefault("float32")
.InEnum({"float32", "bfloat16"})
.InEnum({"float32", "bfloat16", "int8"})
Copy link
Contributor

Choose a reason for hiding this comment

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

The ordering has been usually float32, int8, bfloat16, so what do you think about using that order?
image

paddle/fluid/framework/ir/mkldnn/cpu_quantize_pass.cc Outdated
GET_IR_NODE_FROM_SUBGRAPH(next_op, next_op, slice_pattern);

// skip if prev op and next op is not quantized
if (!(IsOpDequantized(prev_op)) && !(IsOpQuantized(next_op))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think one parenthesis would be enough but it's okay to have them too if you prefer.

@zuzg zuzg dismissed stale reviews from XieYunshen, lidanqing-intel, and wozna via 748a37d November 30, 2021 15:17
sfraczek
sfraczek approved these changes Nov 30, 2021
View reviewed changes
Copy link
Contributor

@sfraczek sfraczek left a comment

Choose a reason for hiding this comment

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

LGTM!

wozna
wozna approved these changes Dec 1, 2021
View reviewed changes
Copy link
Contributor

@wozna wozna left a comment

Choose a reason for hiding this comment

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

LGTM

@wozna
Copy link
Contributor

wozna commented Dec 2, 2021

@XieYunshen Could you please review again?

lidanqing-intel
lidanqing-intel approved these changes Dec 3, 2021
View reviewed changes
Copy link
Contributor

@lidanqing-intel lidanqing-intel left a comment

Choose a reason for hiding this comment

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

LGTM

@lidanqing-intel
Copy link
Contributor

lidanqing-intel commented Dec 3, 2021 •
edited
Loading

@Aganlengzi Hi, could you please help approve this PR, this PR does not add any new attribute, just added one possible value for existing attribute: "mkldnn_data_type".

It was been approved before by Xieyunshen #37630 (review). But cause there is one more reviewe so developer updated code, so it need approval again. Later will avoid similar situations.

@Aganlengzi Aganlengzi self-requested a review December 6, 2021 09:31
@Aganlengzi
Copy link
Contributor

LGTM

@lidanqing-intel
Copy link
Contributor

lidanqing-intel commented Dec 6, 2021 •
edited
Loading

@Aganlengzi Hi, sorry but this PR need XieYunshen's approval, you can see this in log. Thanks.

0. You must have one QA (XieYunshen(Recommend) or chalsliu) approval for setting parameter RUN_TYPE as EXCLUSIVE, DIST, NIGHTLY, EXCLUSIVE:NIGHTLY or DISTNIGHTLY, or setting TIMEOUT properties.

@Aganlengzi
Copy link
Contributor

@XieYunshen
@lidanqing-intel please wait.

XieYunshen
XieYunshen approved these changes Dec 6, 2021
View reviewed changes
Copy link
Contributor

@XieYunshen XieYunshen left a comment

Choose a reason for hiding this comment

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

LGTM for set_tests_properties(test_analyzer_ernie_int8 PROPERTIES TIMEOUT 120)

@Aganlengzi Aganlengzi merged commit 2bd0f3c into PaddlePaddle:develop Dec 7, 2021
Zjq9409 pushed a commit to Zjq9409/Paddle that referenced this pull request Dec 10, 2021
@zuzg @Zjq9409
Quantize slice op (PaddlePaddle#37630)
887ecb8 
* quantize slice op

* correct test

* fix code formatting
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lidanqing-intel lidanqing-intel lidanqing-intel approved these changes

@wozna wozna wozna approved these changes

@XieYunshen XieYunshen XieYunshen approved these changes

@sfraczek sfraczek sfraczek approved these changes

@Aganlengzi Aganlengzi Aganlengzi approved these changes

Assignees

@Aganlengzi Aganlengzi

Labels
int8 Intel
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@zuzg @lidanqing-intel @wozna @Aganlengzi @sfraczek @XieYunshen