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

[NPU] Support NPU kernel for TopKV2 op #34599

Merged
merged 11 commits into from
Aug 11, 2021

Conversation

From00
Copy link
Contributor

@From00 From00 commented Aug 4, 2021

PR types

New features

PR changes

OPs

Describe

Develop the NPU kernel for TopKV2 op and reuse the Python unit test for CPU kernel to test the new kernel.

Unit test result
image

NPU op call result
image

@CLAassistant
Copy link

CLAassistant commented Aug 4, 2021

CLA assistant check
All committers have signed the CLA.

@paddle-bot-old
Copy link

paddle-bot-old bot commented Aug 4, 2021

✅ This PR's description meets the template requirements!
Please wait for other CI results.

@paddle-bot-old
Copy link

paddle-bot-old bot commented Aug 4, 2021

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

@@ -0,0 +1,250 @@
/* Copyright (c) 2016 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.

2016 -> 2021

template <typename T>
class TopkV2NPUKernel : public framework::OpKernel<T> {
public:
// Use Ascend TopKV2 operator to implement paddle TopKV2Op
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this comment is not literally necessary since it is easy for the reader to understand.

public:
// Use Ascend TopKV2 operator to implement paddle TopKV2Op
void Compute(const framework::ExecutionContext& context) const override {
// Read message from context
Copy link
Contributor

Choose a reason for hiding this comment

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

Same above.

indices->mutable_data<int64_t>(context.GetPlace());

// Allocate space for output indices of Ascend topkV2 operator
framework::Tensor* indices_int32 =
Copy link
Contributor

Choose a reason for hiding this comment

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

No related delete?

new Tensor(framework::proto::VarType::INT32);
indices_int32->Resize(output_dims);
indices_int32->mutable_data<int32_t>(context.GetPlace());
VLOG(4) << "input:" << *input;
Copy link
Contributor

Choose a reason for hiding this comment

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

LOG the tensor may cost much time.

.AddAttr("dim", axis)
.AddAttr("largest", largest)
.Run(npu_stream_topkv2);
VLOG(4) << "output:" << *out;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same above.

Comment on lines 89 to 91
auto npu_stream_cast =
context.template device_context<paddle::platform::NPUDeviceContext>()
.stream();
Copy link
Contributor

Choose a reason for hiding this comment

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

It is exactly the same stream with npu_stream_topkv2

Copy link
Contributor

@zhiqiu zhiqiu left a comment

Choose a reason for hiding this comment

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

LGTM

@From00 From00 requested a review from zhiqiu August 10, 2021 15:15
@zhiqiu zhiqiu merged commit bb01b12 into PaddlePaddle:develop Aug 11, 2021
@From00 From00 deleted the add_npu_op_tok_k_v2 branch September 18, 2021 07:56
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.

3 participants