[phi] Transfer lgamma, kldiv_loss, isclose, cumprod kernels into phi and pass the tests of these four kernels #39770

2742195759 · 2022-02-21T06:48:29Z

PR types

Others

PR changes

Others

Describe

[phi] Transfer lgamma, kldiv_loss, isclose, cumprod kernels into phi and pass the tests of these four kernels

paddle-bot-old · 2022-02-21T06:48:33Z

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

YuanRisheng · 2022-02-22T03:02:43Z

paddle/phi/kernels/gpu/lgamma_kernel.cu

+// limitations under the License.
+
+#include <unsupported/Eigen/SpecialFunctions>
+#include "paddle/fluid/operators/elementwise/elementwise_op_impl.cu.h"


这里可以使用pten下的头文件

YuanRisheng · 2022-02-22T03:03:07Z

paddle/phi/kernels/gpu/lgamma_kernel.cu

+  std::vector<const DenseTensor*> ins = {&x};
+  std::vector<DenseTensor*> outs = {out};
+  auto functor = CudaLgammaFunctor<T>();
+  paddle::operators::LaunchSameDimsElementwiseCudaKernel<T>(


参考一下这个函数的实现，其实调用的是Pten下的elementwise组件

YuanRisheng · 2022-02-22T03:05:28Z

paddle/fluid/operators/lgamma_op.cc

@@ -12,7 +12,12 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.

-#include "paddle/fluid/operators/lgamma_op.h"
+#include <unsupported/Eigen/SpecialFunctions>


这个是多余的头文件吗

YuanRisheng · 2022-02-22T03:07:09Z

paddle/fluid/operators/lgamma_op.cc

+#include "paddle/fluid/framework/infershape_utils.h"
+#include "paddle/fluid/framework/op_registry.h"
+#include "paddle/fluid/framework/operator.h"
+#include "paddle/fluid/platform/for_range.h"


这个是多余的头文件吗

YuanRisheng · 2022-02-22T03:19:06Z

paddle/phi/kernels/impl/lgamma_grad.h

+  const DenseTensor* d_out = &d_out_tmp;
+  const DenseTensor* x = &x_tmp;


这里操作有些多余吧，建议参数名改一下，去掉tmp这种后缀，函数体直接改使用 . 操作符就行

YuanRisheng · 2022-03-07T11:34:46Z

paddle/phi/common/scalar.h

@@ -156,6 +160,10 @@ class ScalarBase {
    CopyScalar(other, this);
  }

+  // NOTE(xiongkun): some op need to judge the dtype of the Scalar, we expose a
+  // interface.
+  bool is_dype(DataType d) const { return d == dtype_; }


和其他接口保持统一，使用驼峰命名

YuanRisheng · 2022-03-07T11:35:26Z

paddle/phi/kernels/cpu/cumprod_grad_kernel.cc

+
+#include "paddle/phi/kernels/cumprod_grad_kernel.h"
+
+#include "paddle/fluid/platform/for_range.h"


for_range已迁到phi下

YuanRisheng · 2022-03-07T11:36:06Z

paddle/phi/kernels/cpu/cumprod_kernel.cc

+#include <cstdint>
+#include <type_traits>
+
+#include "paddle/fluid/platform/for_range.h"


YuanRisheng · 2022-03-07T11:36:30Z

paddle/phi/kernels/cpu/cumprod_kernel.cc

+#include "paddle/fluid/platform/for_range.h"
+#include "paddle/phi/backends/cpu/cpu_context.h"
+#include "paddle/phi/core/kernel_registry.h"
+#include "paddle/phi/kernels/cumprod_kernel.h"


这个头文件可以放置在最上边

YuanRisheng · 2022-03-07T11:37:33Z

paddle/phi/kernels/cpu/lgamma_kernel.cc

+// limitations under the License.
+
+#include "paddle/phi/kernels/lgamma_kernel.h"
+#include "paddle/fluid/platform/for_range.h"


YuanRisheng · 2022-03-07T11:39:20Z

paddle/phi/kernels/gpu/cumprod_kernel.cu

+#include "paddle/phi/kernels/cumprod_kernel.h"
+#include "paddle/phi/kernels/funcs/cumprod.h"
+#include "paddle/fluid/operators/math/inclusive_scan.h"
+#include "paddle/fluid/platform/for_range.h"


YuanRisheng · 2022-03-07T11:41:32Z

paddle/phi/kernels/impl/isclose_kernel_impl.h

+#include <string>
+
+#include "paddle/fluid/framework/data_type.h"
+#include "paddle/fluid/framework/eigen.h"


这个头文件有用到吗

YuanRisheng · 2022-03-07T11:41:58Z

paddle/phi/kernels/impl/kldiv_loss.h

+
+#pragma once
+#include <string>
+#include "paddle/fluid/framework/eigen.h"


使用phi下的eigen

YuanRisheng · 2022-03-07T11:42:22Z

paddle/phi/kernels/impl/kldiv_loss_grad.h

+
+#pragma once
+#include <string>
+#include "paddle/fluid/framework/eigen.h"


YuanRisheng · 2022-03-07T11:42:40Z

paddle/phi/kernels/impl/lgamma_grad.h

+// limitations under the License.
+
+#pragma once
+#include "paddle/fluid/platform/for_range.h"


zyfncg · 2022-03-11T11:41:05Z

paddle/fluid/operators/kldiv_loss_op_npu.cc

@@ -12,7 +12,9 @@ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 See the License for the specific language governing permissions and
 limitations under the Licnse. */

-#include "paddle/fluid/operators/kldiv_loss_op.h"
+#include <string>
+#include "paddle/fluid/framework/eigen.h"


npu的kernel是不是不需要eigen？

zyfncg · 2022-03-11T11:52:10Z

paddle/phi/common/scalar.cc

+// NOTE(xiongkun): why we put definition here?
+// test_custom_op can't include enforce.h, because enforce.h includes gflags.
+// so we decouple the include dependence of enforce.h by link.
+void ThrowErrorIfNotScalar(int num) {


这里函数名可以稍微调整下，看函数功能主要是抛出一个Tensor无法转换为Scalar的异常信息，比如可以用ThrowTensorConvertError之类的命名

zyfncg · 2022-03-11T11:58:31Z

paddle/phi/common/scalar.h

@@ -156,6 +155,10 @@ class ScalarBase {
    CopyScalar(other, this);
  }

+  // NOTE(xiongkun): some op need to judge the dtype of the Scalar, we expose a
+  // interface.
+  bool IsDtype(DataType d) const { return d == dtype_; }


目前Scalar已经有一个dtype()接口获取dtype，也可以用来判断，这个IsDtype接口不算是一个常见的类调用接口，非必须的情况下建议移除

zyfncg · 2022-03-11T12:02:59Z

paddle/phi/kernels/cpu/cumprod_kernel.cc

+
+#include <cstdint>
+#include <type_traits>
+#include "paddle/fluid/platform/for_range.h"


引用phi下的for_range.h

zyfncg · 2022-03-11T12:05:53Z

paddle/phi/kernels/cpu/cumprod_kernel.cc

+#include "paddle/phi/kernels/funcs/cumprod.h"
+
+namespace phi {
+using Tensor = DenseTensor;


using Tensor = DenseTensor;可以去掉，目前phi目录下不建议使用这种方式

zyfncg · 2022-03-11T12:19:45Z

paddle/phi/kernels/isclose_kernel.h

@@ -0,0 +1,31 @@
+


多了空行

zyfncg · 2022-03-11T12:19:57Z

paddle/phi/kernels/kldiv_loss_grad_kernel.h

@@ -0,0 +1,30 @@
+


多了空行

zyfncg · 2022-03-11T12:20:40Z

paddle/phi/kernels/kldiv_loss_kernel.h

+                     const DenseTensor& label,
+                     const std::string& reduction,
+                     DenseTensor* out);
+}  // phi


// namespace phi

zyfncg · 2022-03-11T12:20:45Z

paddle/phi/kernels/lgamma_grad_kernel.h

+                      const DenseTensor& d_out,
+                      const DenseTensor& x,
+                      DenseTensor* d_x);
+}  // pten


// namespace phi

zyfncg · 2022-03-11T12:20:51Z

paddle/phi/kernels/lgamma_kernel.h

+void LgammaKernel(const Context& dev_ctx,
+                  const DenseTensor& x,
+                  DenseTensor* out);
+}  // pten


// namespace phi

zyfncg · 2022-03-12T04:27:59Z

paddle/phi/kernels/gpu/cumprod_kernel.cu

+#include "paddle/fluid/operators/math/inclusive_scan.h"
+#include "paddle/phi/backends/gpu/gpu_context.h"
+#include "paddle/phi/core/kernel_registry.h"
+#include "paddle/phi/kernels/cumprod_kernel.h"


cumprod_kernel.h放在开头

zyfncg · 2022-03-12T04:28:21Z

paddle/phi/kernels/gpu/kldiv_loss_grad_kernel.cu

+#include "paddle/phi/backends/gpu/gpu_context.h"
+#include "paddle/phi/core/kernel_registry.h"
+#include "paddle/phi/kernels/impl/kldiv_loss_grad_kernel_impl.h"
+#include "paddle/phi/kernels/kldiv_loss_grad_kernel.h"


kldiv_loss_grad_kernel.h放在开头

zyfncg · 2022-03-12T04:28:33Z

paddle/phi/kernels/gpu/kldiv_loss_kernel.cu

+#include "paddle/phi/backends/gpu/gpu_context.h"
+#include "paddle/phi/core/kernel_registry.h"
+#include "paddle/phi/kernels/impl/kldiv_loss_kernel_impl.h"
+#include "paddle/phi/kernels/kldiv_loss_kernel.h"


zyfncg · 2022-03-12T04:28:43Z

paddle/phi/kernels/gpu/lgamma_grad_kernel.cu

+#include "paddle/phi/backends/gpu/gpu_context.h"
+#include "paddle/phi/core/kernel_registry.h"
+#include "paddle/phi/kernels/impl/lgamma_grad_kernel_impl.h"
+#include "paddle/phi/kernels/lgamma_grad_kernel.h"


zyfncg · 2022-03-12T04:28:51Z

paddle/phi/kernels/gpu/lgamma_kernel.cu

+#include "paddle/phi/backends/gpu/gpu_context.h"
+#include "paddle/phi/core/kernel_registry.h"
+#include "paddle/phi/kernels/funcs/elementwise_base.h"
+#include "paddle/phi/kernels/lgamma_kernel.h"


zyfncg · 2022-03-12T04:34:19Z

paddle/phi/ops/compat/isclose_sig.cc

+  std::string atol_map_str = (ctx.HasInput("Atol") ? "Atol" : "atol");
+  return KernelSignature("isclose",
+                         {"Input", "Other"},
+                         {rtol_map_str, atol_map_str, "equal_nan"},


按照推理目前的设计方案，KernelSignature中的变量需要是原始的变量名字符串，即"Rtol"或者"rtol"，rtol_map_str这里解析会有问题，这里还是得写成最原始的if else判断分别返回

XiaoguangHu01

LGTM

2742195759 added 3 commits February 21, 2022 05:19

tranfer and pass the lgamma unittest

3fd4b9c

Merge remote-tracking branch 'upstream/develop' into transfer_lgamma

5905f5f

merge and pass the test

ead4adb

transfer kldiv_loss and kldiv_loss_grad; pass the unitest

a47ecef

YuanRisheng reviewed Feb 22, 2022

View reviewed changes

2742195759 added 5 commits February 24, 2022 03:48

trafer the isclose and cumprod kernel

5c77ab4

merge

7a76db6

change PT_REGISTER -> PD_REGISTER

64b915d

Merge remote-tracking branch 'upstream/develop' into transfer_lgamma

93dae1d

fix by code review

8f55c8d

2742195759 changed the title ~~Transfer lgamma~~ [phi] Transfer lgamma, kldiv_loss, isclose, cumprod kernels into phi and pass the tests of these four kernels Feb 25, 2022

2742195759 added 4 commits February 25, 2022 10:07

fix by code review

f5a5d1e

Merge remote-tracking branch 'upstream/develop' into transfer_lgamma

606a28f

Merge remote-tracking branch 'upstream/develop' into transfer_lgamma

c9d63fb

fix

7d6f831

YuanRisheng reviewed Mar 7, 2022

View reviewed changes

Merge remote-tracking branch 'upstream/develop' into transfer_lgamma

09ec371

2742195759 marked this pull request as draft March 9, 2022 06:24

2742195759 marked this pull request as ready for review March 9, 2022 06:25

2742195759 marked this pull request as draft March 9, 2022 06:31

2742195759 marked this pull request as ready for review March 9, 2022 06:32

2742195759 added 3 commits March 10, 2022 15:18

remove enforce include dependence from scalar

6e03048

merge, split scalar.h and fix by code review

7b57eb8

fix

36b42f1

zyfncg reviewed Mar 11, 2022

View reviewed changes

fix by code review

dbbeafe

zyfncg reviewed Mar 12, 2022

View reviewed changes

fix by code review

310578d

zyfncg approved these changes Mar 14, 2022

View reviewed changes

YuanRisheng approved these changes Mar 15, 2022

View reviewed changes

XiaoguangHu01 approved these changes Mar 15, 2022

View reviewed changes

Aurelius84 merged commit 6422362 into PaddlePaddle:develop Mar 15, 2022

2742195759 deleted the transfer_lgamma branch March 16, 2022 07:33

		const DenseTensor* d_out = &d_out_tmp;
		const DenseTensor* x = &x_tmp;


		#include "paddle/phi/kernels/cumprod_grad_kernel.h"

		#include "paddle/fluid/platform/for_range.h"

[phi] Transfer lgamma, kldiv_loss, isclose, cumprod kernels into phi and pass the tests of these four kernels #39770

[phi] Transfer lgamma, kldiv_loss, isclose, cumprod kernels into phi and pass the tests of these four kernels #39770

Conversation

2742195759 commented Feb 21, 2022 • edited Loading

PR types

PR changes

Describe

paddle-bot-old bot commented Feb 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XiaoguangHu01 left a comment

Choose a reason for hiding this comment

2742195759 commented Feb 21, 2022 •

edited

Loading