[PHI] traspose2 kernel migration #47748
Conversation
|
你的PR提交成功，感谢你对开源项目的贡献! |
|
|
| // limitations under the License. | ||
|
|
||
| #include "paddle/phi/kernels/transpose_kernel.h" | ||
| #include "paddle/fluid/framework/tensor_util.h" |
There was a problem hiding this comment.
If you use copy function in PHI, please change to copy in "paddle/phi/core/tensor_utils.h"
There was a problem hiding this comment.
@YuanRisheng There is no Copy equivalent for OneDNNContext, should I leave it like this or create OneDNNContext version in paddle/phi/core/tensor_utils.cc? I tried to do it in this PR but then Copy behaved inconsistently with OneDnn
There was a problem hiding this comment.
@paulinagacek Oh, I see. The code of MKLDNN Copy is not implemented in paddle/phi/core/tensor_utils.cc. You may need migrate the code of MKLDNN Copy(The code is in TensorCopyImpl function) from paddle/fluid/framework/tensor_util.cc to paddle/phi/core/tensor_utils.cc. This is a necessary work for MKLDNN kernel migration. Thank you.
There was a problem hiding this comment.
@YuanRisheng I think the problem with phi::Copy may have occured in previous PRs (e.g. transpose2_grad) and there TensorCopy was used instead of phi::Copy, so if I add OneDNNContext version I shoul also change it in other kernels. Can I leave TensorCopy in this PR and dedicate seperate PR for creating OneDNN version of phi::Copy?
There was a problem hiding this comment.
@YuanRisheng so could you give me approve as the check failed because that TensorCopy usage? Could you also take a look at CE-FRAMEWORK please? It fails even though there are no errors and all tests pass. Besides these two checks I think this PR is ready to merge. Thanks!
There was a problem hiding this comment.
@paulinagacek The CE-FRAMEWORK seems have a error. It is a OneDNN error that is unfamiliar to me. I am not sure whether this error is related to the file "operator_reshape2_onednn_fuse_pass.cc" you modified. Maybe you can rerun the CE-FRAMEWORK second time. If CE-FRAMEWORK fails again, you need deal with this issue. After all CI pass(except CI-APPROVAL), I will approve and merge this PR. Thanks.
There was a problem hiding this comment.
@YuanRisheng now tests pass except coverage, because kernel is probably not fully covered by unit tests in fluid. May I get an approve for TensorCopy please?
There was a problem hiding this comment.
That's OK and please create OneDNN version of phi::Copy in next PR. Thank you
|
@YuanRisheng, @chenwhql should I delete transpose and transpose_grad kernels from fluid as they are replaced by tranpose2 and tranpose2_grad in phi or just leave it the way it is? I mean deleting this file: paddle/fluid/operators/mkldnn/transpose_mkldnn_op.cc |
transpose and transpose_grad in Fluid may be used for compatible with old operators. You can just leave it the way it is. |
|
@YuanRisheng why did you close this PR? |
Sorry,I accidentally clicked on the wrong button. |
… transpose2_migration
… transpose2_migration
… transpose2_migration
… transpose2_migration
… transpose2_migration
| // limitations under the License. | ||
|
|
||
| #include "paddle/phi/kernels/transpose_kernel.h" | ||
| #include "paddle/fluid/framework/tensor_util.h" |
There was a problem hiding this comment.
That's OK and please create OneDNN version of phi::Copy in next PR. Thank you
* traspose2 kernel migrated * Got rid of mutable_data * x modification added * ops added in extra info file * Formatting fix * 2 fuse passes with tanpose2 commented * nr of outs changed in 2 passes, passes uncommented * Changes in passes reverted * transpose chnaged in operator.cc * MKLDNN check in operator.cc * Transpose fixes * Fix deleted from operato * template corrected Co-authored-by: Paulina Gacek <[email protected]>
PR types
Others
PR changes
Others
Describe