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 isnan_v2、isfinite_v2、isinf_v2 to phi #40076

Merged
merged 16 commits into from
Mar 8, 2022

Conversation

wjj19950828
Copy link
Contributor

@wjj19950828 wjj19950828 commented Mar 2, 2022

PR types

Function optimization

PR changes

OPs

Describe

迁移phi算子:isnan_v2、isfinite_v2、isinf_v2

1、包括两个kernel的实现:densetensor以及selectedrows
2、将selected_rows kernel相关的声明头文件调整到selected_rows目录下
3、多加一层传入functor

@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,95 @@
// 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.

我看了下底层的TensorContainsInfV2这几个实现,还是属于cpu和gpu公用的实现,这样的话,这个还是属于应该在cpu和gpu子目录下分别创建cc和cu文件的情况,kernels根目录下是放置纯设备无关的kernel实现,比如说这个也需要在xpu和将来的其他设备上能用才行,目前的实现是不行的

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chenwhql Done.已在cpu和gpu子目录下分别创建cc和cu文件

Comment on lines 44 to 54
#define DEFINE_ISFINITE_SR(isfinite_sr, functor) \
template <typename T, typename Context> \
void isfinite_sr( \
const Context& ctx, const SelectedRows& x, SelectedRows* out) { \
IsfiniteSRImpl<T, Context, functor>(ctx, x, out); \
}

DEFINE_ISFINITE_SR(IsinfSR, funcs::InfinityV2Functor)
DEFINE_ISFINITE_SR(IsnanSR, funcs::NANV2Functor)
DEFINE_ISFINITE_SR(IsfiniteSR, funcs::IsfiniteV2Functor)
#undef DEFINE_ISFINITE_SR
Copy link
Contributor

Choose a reason for hiding this comment

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

SelectedRows相关的kernel逻辑需要放在selected_rows目录下

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.

Comment on lines 32 to 40
#define DEFINE_ISFINITE_SR(isfinite_sr) \
template <typename T, typename Context> \
void isfinite_sr( \
const Context& ctx, const SelectedRows& x, SelectedRows* out);

DEFINE_ISFINITE_SR(IsinfSR)
DEFINE_ISFINITE_SR(IsnanSR)
DEFINE_ISFINITE_SR(IsfiniteSR)
#undef DEFINE_ISFINITE_SR
Copy link
Contributor

Choose a reason for hiding this comment

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

同上

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.

Copy link
Contributor

@MingMingShangTian MingMingShangTian left a comment

Choose a reason for hiding this comment

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

LGTM

YuanRisheng
YuanRisheng previously approved these changes Mar 3, 2022
zyfncg
zyfncg previously approved these changes Mar 3, 2022
@@ -0,0 +1,39 @@
// 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.

这个头文件应该是不需要的

namespace phi {

template <typename T, typename Context, typename Functor>
inline void IsfiniteSRImpl(const Context& dev_ctx,
Copy link
Contributor

Choose a reason for hiding this comment

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

SelectedRows在这几个kernel上,建议直接调用DenseTensor的kernel,而不是单独为它实现一个类似的,参考现有的几个SelectedRows kernel

#include "paddle/phi/core/kernel_registry.h"
#include "paddle/phi/kernels/selected_rows/isfinite_kernel_impl.h"

namespace phi {
Copy link
Contributor

Choose a reason for hiding this comment

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

selected_rows目录下命名空间加一层sr,kernel名也不需要加SR的中缀了,参考其他几个SelectedRows kernel

@@ -14,39 +14,32 @@

#pragma once

#include <vector>

#include "paddle/fluid/framework/eigen.h"
#include "paddle/fluid/framework/op_registry.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

op_registry这个头文件还需要吗

Copy link
Contributor

@chenwhql chenwhql left a comment

Choose a reason for hiding this comment

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

几处comment麻烦追加PR完善

@chenwhql chenwhql merged commit 3c536f2 into PaddlePaddle:develop Mar 8, 2022
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