Fix to #38126 #39097
Fix to #38126 #39097
Conversation
|
Thanks for your contribution! |
|
@baoachun Please review |
| const std::string& var_name, const Tensor& tensor, | ||
| const framework::OpKernelType& expected_kernel_type) const { | ||
| #ifdef PADDLE_WITH_MKLDNN | ||
| // When activation is first oneDNN op (there was some non oneDNN op |
There was a problem hiding this comment.
Do all operators need to adapt to the format of the input NHWC when it is the first mkldnn operator?
There was a problem hiding this comment.
Yes. So this PR is only doing this change for fixing #38126 , but in general similar change has to be added to other oneDNN ops.
There was a problem hiding this comment.
Can this be added to a class that other operators inherit?
|
@baoachun I have a problem with CI 👍 |
|
Hi @jczaja , it may be the compilation failure caused by other people's pr. I have triggered PR-CI-Build. If it still fails, I will contact my colleagues to help solve it. |
|
@Silv3S , @piotrekobiIntel Please help with review |
| const std::string& var_name, const Tensor& tensor, | ||
| const framework::OpKernelType& expected_kernel_type) const { | ||
| #ifdef PADDLE_WITH_MKLDNN | ||
| // When activation is first oneDNN op (there was some non oneDNN op |
There was a problem hiding this comment.
Can this be added to a class that other operators inherit?
|
LGTM |
|
@baoachun Please continue your review |
|
@piotrekobiIntel Thanks for suggesstion. Technically yes but in practice we need to create another of class to fit this code there. Code is a bit diffrent for operators with parameters and may differ even for parameter-less ops due to some input and output vars having diffrent names. So in fact adding another class sounds like significant architectural effort while at least half of ops will override this code anyway due to diffrences. So for now I will leave it like this and when more ops got similar code then We can judge if it is profitable to make a separate class with this code. |
PR types
Bug fixes
PR changes
OPs
Describe
This PR is fixing #38126 . In particular it fixes braodcasting of elementwise operation of oneDNN kernels for NHWC scenario.