Skip to content

Conversation

@LLee233
Copy link
Contributor

@LLee233 LLee233 commented Mar 21, 2024

PR types

New features

PR changes

Others

Description

Based on new pass mechanism, here we add pass "matmul_activation_fuse_pass" for PIR.

The new pass is same as "matmul_activation_mkldnn_fuse_pass" in "/paddle/fluid/framework/ir/mkldnn/matmul_activation_mkldnn_fuse_pass.cc"

@paddle-bot
Copy link

paddle-bot bot commented Mar 21, 2024

你的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 the contributor External developers label Mar 21, 2024
@LLee233 LLee233 added the Intel label Mar 21, 2024
@LLee233 LLee233 force-pushed the matmul_act_pass branch 4 times, most recently from e26289f to ff633b9 Compare March 25, 2024 07:45
@LLee233
Copy link
Contributor Author

LLee233 commented Mar 26, 2024

Hi @onecatcn,这个PR的CI已经过了,可以麻烦帮忙请相应的owner来review一下吗?Thx~

return true;
});
pat.RequireNativeCall([&](const paddle::drr::MatchContext &match_ctx) {
auto result_gelu = match_ctx.Attr<bool>("approximate");
Copy link
Contributor

Choose a reason for hiding this comment

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

approximate为false就不能匹配吗?

Copy link
Contributor Author

@LLee233 LLee233 Mar 26, 2024

Choose a reason for hiding this comment

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

我把两种Gelu的情况拆开来写了,这个是新加的一个MatmulGeluTanhPattern,用来单独表示approximate为true时可以fuse的pattern,approximate 为false的时候是跟MatmulActivationFusePattern一起的,会进matmul+gelu_erf fusion。

pir::RewritePatternSet ps(context);
// std::vector<bool> bool_set = {false, true};
int benefit_idx = 1;
for (auto act_op : act_ops) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
for (auto act_op : act_ops) {
for (const auto& act_op : act_ops) {

另外关于benefit_idx的设置可能有些问题,从代码逻辑上看,从上到下依次添加的这些pattern,他们的benefit_idx是依次变大的,benefit_idx值越大的pattern,我们期望是越优先被应用,以MatmulClipFusePattern和FusedMatmulClipFusePattern来说,我们期望MatmulClipFusePattern比FusedMatmulClipFusePattern先应用,因此MatmulClipFusePattern的benefit_idx应该较大才对

Copy link
Contributor Author

Choose a reason for hiding this comment

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

从我的理解来看,因为这个pass目前matmul相关pass里不属于第一个pass,也就是说,在之后遇到fused_matmul的可能性较大,所以这种情况下优先match fused_matmul会不会更好一点?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

不过如果是以单个pattern为准在整个graph上取多个符合要求的pattern,那确实应该是原生matmul+act被match到的个数更多的可能性较大

Copy link
Contributor

Choose a reason for hiding this comment

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

目前这样也可以,问题不大

Copy link
Contributor Author

Choose a reason for hiding this comment

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

好的,那我就先不改了哈。如果之后pass order有变化或者说还是需要以非fused_matmul的pattern为主我再随时更改~

paddle::dialect::MatmulOp::name(),
paddle::onednn::dialect::FusedMatmulOp::name(),
benefit_idx++));
for (auto act_op : act_ops) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
for (auto act_op : act_ops) {
for (const auto& act_op : act_ops) {

Copy link
Contributor

@yuanlehome yuanlehome left a comment

Choose a reason for hiding this comment

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

LGTM

@LLee233
Copy link
Contributor Author

LLee233 commented Mar 27, 2024

Hi @wanghuancoder , @XiaoguangHu01 可以麻烦帮忙review/approve一下吗?Thx~

@yuanlehome yuanlehome merged commit d8f934a into PaddlePaddle:develop Mar 29, 2024
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.

5 participants