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

Enable fc passes #45704

Merged
merged 25 commits into from
Nov 9, 2022
Merged

Enable fc passes #45704

merged 25 commits into from
Nov 9, 2022

Conversation

paulinagacek
Copy link
Contributor

PR types

Others

PR changes

APIs

Describe

It enables following fc mkldnn passes by default when mkldnn is enabled:

  • "fc_mkldnn_pass",
  • "fc_act_mkldnn_fuse_pass",
  • "fc_elementwise_add_mkldnn_fuse_pass".

There was added DisableMkldnnFcPasses() in AnalysisConfig to disable mentioned fc mkldnn passes.

@paddle-bot
Copy link

paddle-bot bot commented Sep 2, 2022

你的PR提交成功,感谢你对开源项目的贡献!
请关注后续CI自动化测试结果,详情请参考Paddle-CI手册
Your PR has been submitted. Thanks for your contribution!
Please wait for the result of CI firstly. See Paddle CI Manual for details.

@paddle-bot paddle-bot bot added contributor External developers status: proposed labels Sep 2, 2022
@CLAassistant
Copy link

CLAassistant commented Sep 2, 2022

CLA assistant check
All committers have signed the CLA.

@paulinagacek paulinagacek marked this pull request as ready for review September 13, 2022 05:34
sfraczek
sfraczek previously approved these changes Sep 13, 2022
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 but let's wait for jenkins results

sfraczek
sfraczek previously approved these changes Sep 29, 2022
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 previously approved these changes Oct 4, 2022
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

@paulinagacek paulinagacek dismissed stale reviews from wozna and sfraczek via 514ebbb October 4, 2022 12:21
sfraczek
sfraczek previously approved these changes Oct 4, 2022
@paulinagacek
Copy link
Contributor Author

@XiaoguangHu01 @jeff41404 @jzhang533 can I get the approve please?

None.
Examples:
.. code-block:: python
from paddle.fluid.core import AnalysisConfig
Copy link
Contributor

Choose a reason for hiding this comment

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

This API is exposed as paddle.inference.Config.disable_mkldnn_fc_passes, right ?

We have a plan to deprecate APIs exposed in paddle.fluid namespace (in python frontend only), @jeff41404 is drafting the plan, and part of this work will happen in next release (v2.5).

So, it would be better to discourage users using paddle.fluid APIs in the sample code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there some other API where it should be put?

Copy link
Contributor

Choose a reason for hiding this comment

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

@sfraczek you can see inference doc for config https://www.paddlepaddle.org.cn/inference/v2.4/api_reference/python_api_doc/Config/ConfigClass.html

import paddle.inference as paddle_infer

paddle_infer.Config

Copy link
Contributor

Choose a reason for hiding this comment

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

@sfraczek

There is a configurable API to disable any Pass paddle.inference.Config.delete_pass(pass: str) , see

https://www.paddlepaddle.org.cn/inference/v2.4/api_reference/python_api_doc/Config/OptimConfig.html#ir

Tt is recommended not to add this API if ‘paddle.inference.Config.delete_pass’ can meet requirements.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@qingqing01
API disable_mkldnn_fc_passes was created to disable all fc passes at one go. They were gathered in the method because they strongly depend on each other and it would have very little sense to disable them separately.
This PR makes fc passes automatically enabled when mkldnn is enabled, so we wanted to give a user the chance to disable them, because previously they were disabled by default.

There are many methods in current API which work the same way, but instead of disabling multiple passes they enable them (e.g. enable_mkldnn_bfloat16, enable_mkldnn_int8) . These methods could have been also replaced with many paddle.inference.Config.insert_pass(pass: str) calls, but due to the strong connection between passes some dedicated methods were created, and we wanted to do the same with paddle.inference.Config.disable_mkldnn_fc_passes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hi @qingqing01 can you please let me know what is the final decision about disable_mkldnn_fc_passes API?

@onecatcn
Copy link
Contributor

onecatcn commented Nov 3, 2022

@yeliang2258 this PR has been blocked for a while, please help to review.

qingqing01
qingqing01 previously approved these changes Nov 7, 2022
@yeliang2258
Copy link
Contributor

@paulinagacek Please resolve the conflict of the code, thanks.

@yeliang2258
Copy link
Contributor

@paulinagacek Hello, I would like to know, why onednn's FC is slower than paddle native in many cases, can this problem be solved completely?

@paulinagacek
Copy link
Contributor Author

@paulinagacek Hello, I would like to know, why onednn's FC is slower than paddle native in many cases, can this problem be solved completely?

@yeliang2258 I will ask my collegue who investigated this issue and I will let you know as soon as possible

@wozna
Copy link
Contributor

wozna commented Nov 8, 2022

Hello, I would like to know, why onednn's FC is slower than paddle native in many cases, can this problem be solved completely?

@yeliang2258
This is not the case in so many cases, it could be an example where the sizes of the tensors are so small that using parallel int8 computation (AVX512_VNNI) will not give us any acceleration and the conversion overhead of fp32 <-> int8 will cause some performance drop.

The most difficult thing about this case is that it depends on the entire model and it is hard to predict what effect this will have on the final performance. We try to repair and optimize most of these common models, but for less popular models the user will have a faster solution thanks to DisableMkldnnFcPasses.

@yeliang2258
Copy link
Contributor

Copy link
Contributor

@yeliang2258 yeliang2258 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@jzhang533 jzhang533 left a comment

Choose a reason for hiding this comment

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

LGTM

@jiangjiajun jiangjiajun merged commit 7e91438 into PaddlePaddle:develop Nov 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contributor External developers Intel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants