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

Replace matmul(v2) with fused_matmul during oneDNN fuse passes #49515

Merged
merged 58 commits into from
Feb 3, 2023

Conversation

Silv3S
Copy link
Member

@Silv3S Silv3S commented Jan 3, 2023

PR types

Others

PR changes

OPs

Describe

  • introduce fused_matmul to handle fusion logic from oneDNN matmul and matmul_v2 fuse passes ,
  • remove fusion logic from base matmul_v2 op and kernel,
  • adjust existing fuse passes and op compat checks.

@paddle-bot
Copy link

paddle-bot bot commented Jan 3, 2023

你的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 Jan 3, 2023
@Silv3S Silv3S added the Intel label Jan 3, 2023
paddle/fluid/operators/fused/fused_matmul_op.cc Outdated Show resolved Hide resolved
paddle/fluid/operators/fused/fused_matmul_op.cc Outdated Show resolved Hide resolved
paddle/fluid/operators/fused/fused_matmul_op.cc Outdated Show resolved Hide resolved
paddle/phi/backends/onednn/onednn_reuse.h Outdated Show resolved Hide resolved
paddle/phi/kernels/fusion/onednn/fused_matmul_kernel.cc Outdated Show resolved Hide resolved
paddle/phi/kernels/fusion/onednn/fused_matmul_kernel.cc Outdated Show resolved Hide resolved
paddle/phi/ops/compat/fused_matmul_sig.cc Outdated Show resolved Hide resolved
sfraczek
sfraczek previously approved these changes Jan 16, 2023
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

paddle/fluid/operators/matmul_v2_op.cc Show resolved Hide resolved
Copy link
Contributor

@tsocha tsocha left a comment

Choose a reason for hiding this comment

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

TODOs

tsocha
tsocha previously approved these changes Jan 18, 2023
@Silv3S Silv3S requested a review from zyfncg January 19, 2023 11:55
@qingqing01 qingqing01 self-requested a review January 31, 2023 03:10
Copy link
Contributor

@qingqing01 qingqing01 left a comment

Choose a reason for hiding this comment

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

LGTM

@qingqing01
Copy link
Contributor

2023-01-18 23:38:58 ****************
2023-01-18 23:38:58 0. You must have Dianhai or XiaoguangHu01 approval for change 20+ files or add than 1000+ lines of content.
2023-01-18 23:38:58 There are 1 approved errors.
2023-01-18 23:38:58 ****************

@raindrops2sea @XiaoguangHu01 please help to review~

@@ -84,6 +84,12 @@ void MatmulTransposeReshapeMKLDNNPass::Fuse(
}

OpDesc *matmul_desc = matmul_op->Op();
matmul_desc->SetType("fused_matmul");
Copy link
Contributor

Choose a reason for hiding this comment

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

There have some broadcast computational logic difference between matmul_v1 and matmul_v2, the code as follows

import paddle 
data1 = paddle.rand((3, 1, 10, 10))
data2 = paddle.rand((1, 2, 10, 10))


# The paddle.matmul (matmul_v2)
paddle.matmul(data1, data2).shape # [3, 2, 10, 10]


# The fluid.layers.matmul (matmul_v1)
paddle.fluid.layers.matmul(data1, data2).shape # 直接报错
# ValueError: (InvalidArgument) The batch size of the two matrices should be equal, or at least one is zero.
# But received X's shape: [3, 10, 10], Y's shape: [2, 10, 10]

Is there some problems when replacing directly ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi, I don't think that it could be problem. For some time oneDNN version of both matmul and matmul_v2 are already using same code #44640, #48162. Declared attributes of these two are mostly the same. Only difference is alpha not supported in matmul_v2, and transpose is renamed to trans).

Goal of this PR is just to extract fusion logic from base op and base kernel. Fused op is superset, which has all extra attributes declared in OPMaker and fused kernel has implemented support for handling fusion logic.

I've adjusted all fuse pass unit test to work with fused_matmul. Also this PR has been checked in our internal validation and it didn't report any accuracy drop.

Copy link
Contributor

@wawltor wawltor 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

@XiaoguangHu01 XiaoguangHu01 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

@Aurelius84 Aurelius84 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 data type Registeration

@qingqing01 qingqing01 merged commit 5cfe164 into PaddlePaddle:develop Feb 3, 2023
@Silv3S Silv3S deleted the remove_extra_matmul_attrs branch February 3, 2023 10:40
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.

8 participants