-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
[PHI] transpose2_grad op migration #46139
[PHI] transpose2_grad op migration #46139
Conversation
你的PR提交成功,感谢你对开源项目的贡献! |
… transpose2_grad_op_migration
…linaGacek/Paddle into transpose2_grad_op_migration
@piotrekobi can you review please? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make some changes for clarity and style:
- Remove phi::funcs i phi:: wherever possible (everywhere apart from the kernel registration at the end). In phi namespace leave only funcs::
- Move the transpose_grad_kernel.h import to the top with a new line in between the other imports (like in https://github.com/PaddlePaddle/Paddle/pull/46051/files)
- Change the kernel file name to transpose2_grad_kernel.cc
… transpose2_grad_op_migration
… transpose2_grad_op_migration
… transpose2_grad_op_migration
… transpose2_grad_op_migration
… transpose2_grad_op_migration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@chenwhql Please review |
|
||
} // namespace phi | ||
|
||
PD_REGISTER_KERNEL( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The transpose2
in Fluid is renamed to transpose
in PHI. So , the registered name should be transpose_grad
and this file should be renamed transpose_grad_kernel.cc
. On the other hand, CI-coverage is not successful, maybe the reason is that the registerd name is wrong.
… transpose2_grad_op_migration
… transpose2_grad_op_migration
… transpose2_grad_op_migration
… transpose2_grad_op_migration
@chenwhql @YuanRisheng PR-CI-Windows tests give weird results, can you please take a look at it? |
std::vector<int> reversed_axis(axis); | ||
int ndims = axis.size(); | ||
if (ndims == 1) { | ||
Copy(dev_ctx, out_grad, out_grad.place(), false, x_grad); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found that phi::Copy
and framework::TensorCopy
behave inconsistently in the mkldnn scenario, you can still use framework::TensorCopy
for the time being, the problem of phi::Copy
I will solve
@paulinagacek the problem of PR-CI-Windows may be caused by the |
… transpose2_grad_op_migration
… transpose2_grad_op_migration
@@ -1076,9 +1076,7 @@ class TransposeOneDNNHandler { | |||
std::shared_ptr<dnnl::memory> AcquireDstMemory(DenseTensor* output, | |||
Place place) { | |||
auto dst_md = Axis2MemoryDesc(dims_, axis_); | |||
output->Resize(make_ddim(dims_)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I deleted Resize() and now it seems to work properly, @chenwhql @YuanRisheng please review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, please take a look at the Coverage CI @paulinagacek
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have checked the Coverage CI in my machine and the code is exercised. I have marked it as successful.
* op migrated, Copy(OneDNNContext, ...) added * mutable_data & op registration in fluid removed * refactoring * OneDNNGetDataType to uppercase * missing cpu check added, handler moved to .h file * name changed to transpose_grad * Copy changed back to TensorCopy * Resizing corrected, Copy(OneDNNContext) removed
* op migrated, Copy(OneDNNContext, ...) added * mutable_data & op registration in fluid removed * refactoring * OneDNNGetDataType to uppercase * missing cpu check added, handler moved to .h file * name changed to transpose_grad * Copy changed back to TensorCopy * Resizing corrected, Copy(OneDNNContext) removed
* Revert pool+grad oneDNN kernel conversion (#45989) * [PHI] transpose2_grad op migration (#46139) * op migrated, Copy(OneDNNContext, ...) added * mutable_data & op registration in fluid removed * refactoring * OneDNNGetDataType to uppercase * missing cpu check added, handler moved to .h file * name changed to transpose_grad * Copy changed back to TensorCopy * Resizing corrected, Copy(OneDNNContext) removed Co-authored-by: Piotr Paturej <[email protected]> Co-authored-by: Paulina Gacek <[email protected]>
PR types
Others
PR changes
Others
Describe