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

[ROCM] update fluid operators for rocm (part3), test=develop #31213

Merged
merged 3 commits into from
Mar 3, 2021

Conversation

qili93
Copy link
Contributor

@qili93 qili93 commented Feb 25, 2021

PR types

New features

PR changes

OPs

Describe

[ROCM] update fluid operators for rocm (part3)

@paddle-bot-old
Copy link

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

GaoWei8
GaoWei8 previously approved these changes Feb 26, 2021
Copy link
Contributor

@GaoWei8 GaoWei8 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

@qili93 qili93 requested a review from luotao1 February 26, 2021 07:55
luotao1
luotao1 previously approved these changes Feb 26, 2021
Copy link
Contributor

@luotao1 luotao1 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 const_cast

@@ -41,7 +41,7 @@ HOSTDEVICE inline int64_t BinarySearch(const T *x, int64_t num, const T &val) {

template <typename T>
HOSTDEVICE inline size_t LowerBound(const T *x, size_t num, const T &val) {
#ifdef __CUDA_ARCH__
#if defined(__CUDA_ARCH__) || defined(__HIPCC__)
Copy link
Contributor

@Superjomn Superjomn Mar 1, 2021

Choose a reason for hiding this comment

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

全局宏 if, elif, else 比较多,中间间隔的代码也比较长,阅读起来比较碎片化
建议加一些注释来让代码片段区间更清晰些,比如

#if defined(__CUDA_ARCH__) || defined(__HIPCC__)  // @ {
...
// @ }
#else // @ {

// @ }

这样跨了多行的宏读起来能对应起来

参考 https://www.doxygen.nl/manual/grouping.html#memgroup ,不确定有没有更好的方式

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, 给跨度比较大的被宏包围的代码添加了注释

@qili93 qili93 dismissed stale reviews from luotao1 and GaoWei8 via 720bcdc March 1, 2021 11:57
@qili93 qili93 requested a review from Superjomn March 1, 2021 11:58
Superjomn
Superjomn previously approved these changes Mar 1, 2021
Copy link
Contributor

@Superjomn Superjomn left a comment

Choose a reason for hiding this comment

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

LGTM

GaoWei8
GaoWei8 previously approved these changes Mar 2, 2021
Copy link
Contributor

@GaoWei8 GaoWei8 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

@qili93 qili93 dismissed stale reviews from GaoWei8 and Superjomn via d8145a7 March 2, 2021 05:18
Copy link
Contributor

@Superjomn Superjomn left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@GaoWei8 GaoWei8 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 benchamrk ci

@qili93 qili93 merged commit 84639b6 into PaddlePaddle:develop Mar 3, 2021
@qili93 qili93 deleted the rocm_operators_part3 branch March 3, 2021 08:34
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.

4 participants