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

Fix for split op in BF16 inference #39548

Merged
merged 4 commits into from
Feb 24, 2022
Merged

Fix for split op in BF16 inference #39548

merged 4 commits into from
Feb 24, 2022

Conversation

jakpiase
Copy link
Contributor

PR types

Bug fixes

PR changes

Others

Describe

Fix for split op in BF16 inference. Bug was related to incorrect handling of multi-output which was resolved in similar way to handling multi-input in ops such as concat.

@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 reviewed Feb 16, 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.

You did a good job. Could you please add a unit test to these changes in cpu_bfloat16_pass._tester.cc?

@lidanqing-intel
Copy link
Contributor

@sfraczek Please help review, thanks !

wozna
wozna previously 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
lidanqing-intel previously 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

sfraczek
sfraczek reviewed Feb 17, 2022
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.

It looks good, however I had some comments.

paddle/fluid/framework/ir/mkldnn/cpu_bfloat16_pass.cc Outdated Show resolved Hide resolved
paddle/fluid/framework/ir/mkldnn/cpu_bfloat16_pass.cc Outdated Show resolved Hide resolved
paddle/fluid/framework/ir/mkldnn/cpu_bfloat16_pass.cc Outdated Show resolved Hide resolved
@jakpiase jakpiase dismissed stale reviews from lidanqing-intel and wozna via b43aeef February 18, 2022 16:12
sfraczek
sfraczek previously approved these changes Feb 18, 2022
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

@jakpiase
Copy link
Contributor Author

@baoachun could you please review this PR?

@lidanqing-intel lidanqing-intel requested review from Aganlengzi and baoachun and removed request for Aganlengzi February 21, 2022 07:25
baoachun
baoachun reviewed Feb 21, 2022
View reviewed changes
paddle/fluid/framework/ir/mkldnn/cpu_bfloat16_pass.cc
void CPUBFloat16Pass::SetOutputDataType(ir::Graph* graph) const {
void AddDequantize(Graph* g, ir::Node* op, ir::Node* op_out,
int& dequantize_counter) {
if (op->Op()->Type() == "prior_box") return;
Copy link
Contributor

@baoachun baoachun Feb 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Why does prior_box return directly? Could you add descriptions or can we use set to maintain operators that require special handling?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wozna could you please advise us on this prior_box scenario?

Copy link
Contributor

@wozna wozna Feb 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Prior_box output always produces floating-point results because these are prior boxes generated. Therefore, we do not need dequantization. And so far only this operator is behaving this way.

Copy link
Contributor

Choose a reason for hiding this comment

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

Jakub may add comments in next PR, but this PR has passed all CIs and hopefully it could be merged

baoachun
baoachun approved these changes Feb 24, 2022
View reviewed changes
Copy link
Contributor

@baoachun baoachun 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
lidanqing-intel approved these changes Feb 24, 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

@lidanqing-intel lidanqing-intel merged commit 75f91ce into PaddlePaddle:develop Feb 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wozna wozna wozna left review comments

@baoachun baoachun baoachun approved these changes

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

@sfraczek sfraczek sfraczek left review comments

Assignees

@baoachun baoachun

Labels
BF16 Intel
Projects
None yet
Milestone
No milestone
Development

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