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

[Phi]Move topk kernel to phi #40064

Merged
merged 20 commits into from
Mar 10, 2022
Merged

[Phi]Move topk kernel to phi #40064

merged 20 commits into from
Mar 10, 2022

Conversation

ZzSean
Copy link
Contributor

@ZzSean ZzSean commented Mar 2, 2022

PR types

Others

PR changes

OPs

Describe

Move topk kernel to phi

@paddle-bot-old
Copy link

paddle-bot-old bot commented Mar 2, 2022

Thanks for your contribution!
Please wait for the result of CI firstly. See Paddle CI Manual for details.

@@ -0,0 +1,61 @@
// Copyright (c) 2022 PaddlePaddle Authors. All Rights Reserved.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里能否试下直接用transpose_kernel.h中的Transpose,phi已经有两套transpose的封装了

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done, thx

template <typename T, typename Context>
void TopkKernel(const Context& dev_ctx,
const DenseTensor& x,
paddle::optional<const DenseTensor&> k_t,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这种需要使用Scalar,而不是保留两个参数,麻烦参考scale kernel的scale参数修改一下

namespace phi {

KernelSignature TopkOpArgumentMapping(const ArgumentMappingContext& ctx) {
return KernelSignature("top_k",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

同上,可以参考scale_sig

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done, thx

@@ -51,6 +51,19 @@ namespace operators {

using Tensor = framework::Tensor;

inline void GetDims(const phi::DDim& dim, int axis, int* pre, int* n,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个文件可以迁到phi的funcs下,虽然代码量较大,但迁移难度应该不大

Copy link
Contributor Author

@ZzSean ZzSean Mar 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

有其他文件依赖这个头文件,如果迁移了这个文件,文件数和改动行数都会超过限制,后面PR再改吧

namespace funcs {

template <typename Context, typename T>
inline void TransCompute(const int dim,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里的TransCompute如果有复用的话可以和现有的Transpose整合一下,如果仅topk kernel使用的话可以放到topk kernel的文件中

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done, thx

Copy link
Contributor

@Xreki Xreki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM for op benchmark ci

@ZzSean ZzSean merged commit 329b095 into PaddlePaddle:develop Mar 10, 2022
@ZzSean ZzSean deleted the move_topk branch April 14, 2022 09:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants