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

Matmul_v2 broadcasting mkldnn fuse passes #36461

Closed
baoachun opened this issue Oct 15, 2021 · 13 comments
Closed

Matmul_v2 broadcasting mkldnn fuse passes #36461

baoachun opened this issue Oct 15, 2021 · 13 comments
Assignees
Labels
Milestone

Comments

@baoachun
Copy link
Contributor

Hi @lidanqing-intel @jczaja, this AR has a higher priority, and ends on the 29th. Thank you very much!

@paddle-bot-old
Copy link

您好,我们已经收到了您的问题,会安排技术人员尽快解答您的问题,请耐心等待。请您再次检查是否提供了清晰的问题描述、复现代码、环境&版本、报错信息等。同时,您也可以通过查看官网API文档常见问题历史IssueAI社区来寻求解答。祝您生活愉快~

Hi! We've received your issue and please be patient to get responded. We will arrange technicians to answer your questions as soon as possible. Please make sure that you have posted enough message to demo your request. You may also check out the APIFAQGithub Issue and AI community to get the answer.Have a nice day!

@lidanqing-intel lidanqing-intel changed the title add matmul_v2 mkldnn related fuse passes [2.2 Final tag] Add matmul_v2 mkldnn related fuse passes Oct 15, 2021
@jakpiase jakpiase assigned jakpiase and unassigned yingyibiao Oct 15, 2021
@baoachun
Copy link
Contributor Author

Hi @lidanqing-intel @jakpiase , I am wondering whether we can add a new pass to map matmul_v2 to v1 to reduce the modification?

@jakpiase
Copy link
Contributor

jakpiase commented Oct 22, 2021

Hi, @baoachun, do mean adding a pass that will convert "matmul_v1" into "matmul_v2"? If yes, then I think that it can be done, but only if alpha(output scale) parameter would be equal to 1.0, otherwise matmul_v2 may not be compatible since it does not have that parameter. This additional pass will be added by your team, or it should be added by mine? Does it have the same deadline as adding other fuses(29.10.2021)?

@baoachun
Copy link
Contributor Author

Hi, @jakpiase, my colleague will submit the pass of v2 mapping v1 today, but it only supports scenes without broadcast. Scenes that cannot be mapped by broadcast are not necessary to be resolved before the deadline. We need to ensure that the model in this mapping scenario can be inferred successfully and the related passes can work normally. In addition, yesterday PMO requested that cherry-pick codes be completely disallowed starting on the 25th. Thank you for your hard support!

@jakpiase
Copy link
Contributor

@baoachun So should I create a cherry-pick PR of matmul_v2+transpose+reshape fuse pass now(#36481 )?

@baoachun
Copy link
Contributor Author

@jakpiase I will ask the leader’s opinion and reply to you on Monday.
In addition, according to my colleague's pr, v2 has been mapped to v1, but ernie inference will have an inconsistent dimension error, when mkldnn is turned on. Can you help me see where the problem might be? The link to pr is: https://github.com/PaddlePaddle/Paddle/pull/36652/files
图片
I did find that the two input shapes of the matmul operator do not match, the correct ones are:

dim_a.height_: 115
dim_a.width_: 64
dim_a.batch_size_: 32

dim_b.height_: 64
dim_b.width_: 115
dim_b.batch_size_: 32

the wrong ones are:

dim_a.height_: 115
dim_a.width_: 115
dim_a.batch_size_: 32

dim_b.height_: 115
dim_b.width_: 1024
dim_b.batch_size_: 2

@jakpiase
Copy link
Contributor

@baoachun Ok, I'll try to find out why is it crashing in that PR

@baoachun
Copy link
Contributor Author

baoachun commented Oct 23, 2021

Hi @jakpiase , I really appreciate it!
I have figured out the problem, only need to pass in the use_mkldnn attribute when mapping. But I don't quite understand why this method can avoid this problem. Do you have any insights?
图片

@jakpiase
Copy link
Contributor

if(matmul_v2_op->Op()->HasAttr("use_mkldnn")) {
  desc.SetAttr("use_mkldnn", matmul_v2_op->Op()->GetAttr("use_mldnn");
} else {
  desc.SetAttr("use_mkldnn", false);
}

Maybe you can use something like that instead of using GetAttrIfExists() method, since GetAttrIfExists() will return uninitialized bool if there is no "use_mkldnn" attribute in matmul_v2 op

@baoachun
Copy link
Contributor Author

Got it, we have made changes.

@jakpiase
Copy link
Contributor

Hi @baoachun, CI-Framework still has some errors. X tensor batch size = "-1", but Y tensor has batch size = "1", and that's matmul_v1, so there can't be any broadcasting, so probably setting proper X tensor dims should fix that
image

@baoachun
Copy link
Contributor Author

baoachun commented Oct 24, 2021

Hi @jakpiase, I have figured out the problem, but there has been ouput diff after I modified the matmul v1 op. Chould you please help me see where the problem might be? Thanks a lot.
My pr link https://github.com/PaddlePaddle/Paddle/pull/36669/files#, the paddle/fluid/operators/matmul_op.cc file. I couldn’t open the github link with my computer, so I just reply to you with my phone.

@lidanqing-intel lidanqing-intel changed the title [2.2 Final tag] Add matmul_v2 mkldnn related fuse passes [Need broadcasting fuse] Add matmul_v2 mkldnn related fuse passes Nov 2, 2021
@lidanqing-intel lidanqing-intel changed the title [Need broadcasting fuse] Add matmul_v2 mkldnn related fuse passes Matmul_v2 broadcasting mkldnn fuse passes Nov 5, 2021
@lidanqing-intel lidanqing-intel added this to the Q4 milestone Nov 5, 2021
@lidanqing-intel
Copy link
Contributor

@baoachun Please share some models with broadcasting so that we could test with models

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants