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

Adding is_test argument to PHI kernels that don't use it #45954

Closed
piotrekobi opened this issue Sep 12, 2022 · 7 comments
Closed

Adding is_test argument to PHI kernels that don't use it #45954

piotrekobi opened this issue Sep 12, 2022 · 7 comments
Assignees

Comments

@piotrekobi
Copy link
Contributor

piotrekobi commented Sep 12, 2022

During migration of some oneDNN kernels to PHI, it turned out that some kernels don't use the is_test variable as an argument. This variable is used in some kernels to make oneDNN perform less computation during inference. I migrated the pool2d kernel without using this variable #45775 but this resulted in a performance decrease. For now the oneDNN kernels we've found to require is_test, but are not provided its value in PHI kernels are pool2d and prelu.
We would like to add this variable to those PHI kernels. This would change the kernel function headers as follows:
From:

template <typename T, typename Context>
void PReluKernel(const Context& dev_ctx,
                 const DenseTensor& x,
                 const DenseTensor& alpha,
                 const std::string& data_format,
                 const std::string& mode,
                 DenseTensor* out);

To:

template <typename T, typename Context>
void PReluKernel(const Context& dev_ctx,
                 const DenseTensor& x,
                 const DenseTensor& alpha,
                 const std::string& data_format,
                 const std::string& mode,
                 bool is_test,
                 DenseTensor* out);

Are we allowed to make this change in required kernels?
Is there anything else that needs to be done apart from adding the variable to the kernel function declarations and implementations?

@paddle-bot
Copy link

paddle-bot bot commented Sep 12, 2022

您好,我们已经收到了您的问题,会安排技术人员尽快解答您的问题,请耐心等待。请您再次检查是否提供了清晰的问题描述、复现代码、环境&版本、报错信息等。同时,您也可以通过查看官网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!

@jczaja
Copy link
Contributor

jczaja commented Sep 12, 2022

@chenwhql Please help with this one

@chenwhql
Copy link
Contributor

This is why we have [Migrate later] OpKernel list

  • Migrate later: 23 in total. This kind of Kernel has the problem of Extra parameters in OpMaker that we mentioned before. It requires more parameters than CPU&GPU Kernel, so this part of Kernel is recommended to be migrated after we complete the extra parameter cleaning.

is_test is not a argument of mathematical meaning in pool2d, it only used for mkldnn, if we add it to the public kernel declaration, it will affects the kernel of all other devices, it is not a good design.

let's also count pool2d and pool2d_grad into [Migrate later] OpKernel list, don't migrate them now.

Now we are building the library of custom device, if we change this declaration in framwork, we also need to change kernel implemention of npu and mlu in PaddleCustomDeviceDevice, like

Such custom device will continue to increase in the future, for other hardware manufacturers, these extra parameters only increase the cost of understanding.

At present, we need a solution to think about how to minimize the impact on other hardware while being compatible with mkldnn. Maybe we have to make a change to the kernel declaration in the future in order to be compatible with mkldnn, but I hope we only change it once, to avoid changing it again in the future.

@piotrekobi
Copy link
Contributor Author

I hope you come up with a good solution. For now I reverted the conversion of the pool+grad kernels: #45989

@paddle-bot paddle-bot bot added status/following-up 跟进中 and removed status/new-issue 新建 labels Sep 13, 2022
@jczaja
Copy link
Contributor

jczaja commented Sep 13, 2022

@chenwhql Thanks for answer, We have (fluid) a mechanism where mkldnn/onednn specific attributes are added in runtime. If this mechanism is available for PHI then is_test could be also added in runtime(during IR passes).

@chenwhql
Copy link
Contributor

Thanks for understanding. I am also thinking of a solution, and I will find a way to solve this problem, and then submit a PR to request review.

@chenwhql
Copy link
Contributor

I hope you come up with a good solution. For now I reverted the conversion of the pool+grad kernels: #45989

Sorry for not classifying poo2d into [Migrate later] OpKernel list correctly before

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

No branches or pull requests

4 participants