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

[bugfix] to concat input squash #39593

Merged
merged 6 commits into from
Feb 17, 2022
Merged

[bugfix] to concat input squash #39593

merged 6 commits into from
Feb 17, 2022

Conversation

sfraczek
Copy link
Contributor

PR types

Bug fixes

PR changes

Others

Describe

Bugfix to bugfix #39346

@paddle-bot-old
Copy link

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

@sfraczek sfraczek changed the title fix and add more tests [bugfix] to concat input squash Feb 15, 2022
@sfraczek
Copy link
Contributor Author

PR-CI-Model-benchmark

	Line 1343: 2022-02-16 02:37:00 Performance of model ResNet50_bs32_dygraph has been decreased from 597.617 to 557.766,rerun in paddle develop
	Line 1367: 2022-02-16 02:37:00 Performance of model yolov3_bs8 has been decreased from 58.377 to 53.547,rerun in paddle develop
	Line 1991: 2022-02-16 02:44:58 Performance of model yolov3_bs8 has been decreased from 57.175 to 53.547,which is greater than threshold.

@sfraczek sfraczek mentioned this pull request Feb 16, 2022
Merged
wozna
wozna reviewed Feb 16, 2022
View reviewed changes
paddle/fluid/framework/ir/mkldnn/cpu_quantize_squash_pass.cc Outdated
if (is_concat_signed && is_input_unsigned) {
VLOG(4) << "Do not squash dequant-quant, because "
<< "is_concat_signed: " << is_concat_signed
<< ", is_input_unsigned: " << is_input_unsigned << ".";
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a check if the next operator is concat or elementwise, because only these operators have several inputs and they all have to have the same data type.
For elementwise check, you can add a comment to remove it when the BinaryMkldnn kernel will support two different data types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@sfraczek
Copy link
Contributor Author

I have reverted fix to PTQ to restore LSTM test to working until we find a way to properly fix it.

@sfraczek sfraczek requested a review from jczaja February 16, 2022 19:52
wozna
wozna approved these changes Feb 17, 2022
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

@lidanqing-intel
Copy link
Contributor

@sfraczek Is Coverage CI randomly failing ?

lidanqing-intel
lidanqing-intel approved these changes Feb 17, 2022
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
This PR fixes yolo_darknet and yolo_mobilenet

@sfraczek
Copy link
Contributor Author

sfraczek commented Feb 17, 2022 •
edited
Loading

@baoachun there is no information why coverage build failed. Only that it's linker error. Can you tell if it's run out of space or not? I did rerun it once already and it still fails. I can open PR that adds verbose flags to linker hoping that it will show why it failed. What do you think?
[edit] now it works

arlesniak
arlesniak approved these changes Feb 17, 2022
View reviewed changes
Copy link
Contributor

@arlesniak arlesniak left a comment

Choose a reason for hiding this comment

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

LGTM

@jczaja jczaja merged commit f29da15 into PaddlePaddle:develop Feb 17, 2022
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

@arlesniak arlesniak arlesniak approved these changes

@jczaja jczaja Awaiting requested review from jczaja

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

Successfully merging this pull request may close these issues.
5 participants
@sfraczek @lidanqing-intel @arlesniak @wozna @jczaja