[PHI]Unify Fluid and PHI kernel #49328

YuanRisheng · 2022-12-26T06:11:33Z

PR types

Others

PR changes

Others

Describe

完善机制，使得Fluid的结构体Kernel可以注册到PHI中，并且修改rank_loss_op验证功能

paddle-bot · 2022-12-26T06:11:37Z

你的PR提交成功，感谢你对开源项目的贡献!
请关注后续CI自动化测试结果，详情请参考Paddle-CI手册。
Your PR has been submitted. Thanks for your contribution!
Please wait for the result of CI firstly. See Paddle CI Manual for details.

jiahy0825 · 2023-01-16T03:53:27Z

paddle/phi/core/kernel_factory.h

@@ -241,10 +241,13 @@ class Kernel {

  bool IsValid() const { return fn_ != nullptr; }

+  bool IsFuncKernel() const { return is_func_kernel_; }


个人理解，函数名改成 IsStructKernel 比较好？
IsFuncKernel 和 StructPhiKernel 是取反的概念，IsFuncKernel 的命名凭空多了一层理解成本

Maybe changing the name to IsStructKernel would be better?
IsFuncKernel and StructPhiKernel are opposite concepts, the name IsFuncKernel makes the understanding cost higher.

具体有没有理解成本应该是从使用角度来考虑，可以看下图使用，并没有增加理解成本

个人感觉StructuredKernel从理解上会比较容易些，torch中也有类似的命名方式，具体可以再讨论下

或许可以两个接口都加上？两种判断都更容易理解？IsFuncKernel 和 IsStructKernel

讨论后决定使用枚举值来判断

jiahy0825 · 2023-01-16T04:01:03Z

paddle/phi/core/kernel_registry.h

+                      FUNCTION_KERNEL_INSTANTIATION,                          \
+                      ARG_PARSE_FUNCTOR,                                      \
+                      PHI_KERNEL,                                             \
+                      PHI_VARIADIC_KERNEL,                                    \


这四个宏的含义？是否有简化的可能？

What's the meaning of these four macros? Is it possible to simplify the registration process?

好的，这里我会追加一下注释，不过简化方式目前我并没有想到更合适的方法，宏展开不太好处理，如果你有比较好的想法可以提一下

…rnel

chenwhql · 2023-01-18T06:14:39Z

paddle/fluid/framework/op_registry.h

@@ -486,3 +487,36 @@ struct OpKernelRegistrarFunctorEx<PlaceType,

 }  // namespace framework
 }  // namespace paddle
+
+namespace phi {


namespace还是在framework里吧，这个和目录是有关的，这里定位给fluid专用，咱们也不需要都搞到phi里

好的，我改一下

chenwhql · 2023-01-18T06:22:15Z

paddle/phi/core/kernel_factory.h

@@ -241,10 +241,13 @@ class Kernel {

  bool IsValid() const { return fn_ != nullptr; }

+  bool IsFuncKernel() const { return is_func_kernel_; }


或许可以两个接口都加上？两种判断都更容易理解？IsFuncKernel 和 IsStructKernel

chenwhql · 2023-01-18T06:29:07Z

paddle/phi/core/kernel_registry.h

@@ -32,7 +32,7 @@
 namespace phi {

 #define BACKEND(arg__) phi::Backend::arg__
-#define DATALAYOUT(arg__) phi::DataLayout::arg__
+#define _DATALAYOUT(arg__) phi::DataLayout::arg__


这个地方加下滑线的考虑是？

由于这个头文件在op_registry.h里进行了引用，造成了和其他模块宏定义冲突的问题，这里加下划线去除冲突，表示内部使用

这种情况是不改op_registry.h里的宏定义名会更合适？

我和其他团队下去再沟通一下吧，这个涉及到其他团队宏定义的命名，咱们不改他们就要改，需要看一下对他们的影响范围

chenwhql · 2023-01-18T06:49:07Z

paddle/phi/core/kernel_registry.h

+                            meta_kernel_fn,                                \
+                            kernel_instantiation,                          \
+                            arg_parse_functor,                             \
+                            to_kernel,                                     \


这里的名称可以再用得更“自解释”一些吗？这里是向下传递一个用于展开kernel的宏，比如是否可以叫“kernel_unfold_macro”, "variadic_kernel_unfold_marco"

chenwhql · 2023-01-18T06:49:58Z

paddle/phi/core/kernel_registry.h

+                            context,                                       \
+                            layout,                                        \
+                            meta_kernel_fn,                                \
+                            kernel_instantiation,                          \


以及这个，kernel_instantiate_macro

好的，谢谢

…rnel

zyfncg · 2023-02-03T03:30:35Z

paddle/phi/core/kernel_factory.cc

+    return std::all_of(kernel_iter->second.begin(),
+                       kernel_iter->second.end(),
+                       [](phi::KernelKeyMap::const_reference kernel_pair) {
+                         return kernel_pair.second.GetKernelRegisteredType() ==
+                                KernelRegisteredType::STRUCTURE;
+                       });


同一个Kernel的map里会同时有STRUCTURE和FUNCTION两种类型的Kernel吗？

一般不会，这里我再改一下吧，在注册端拦截一下Kernel的类型，防止出现多种类型的注册

zyfncg · 2023-02-03T03:36:48Z

paddle/phi/core/kernel_registry.h

+    if (variadic_kernel_fn == nullptr) {
+      kernel_registered_type = KernelRegisteredType::STRUCTURE;
+    }


看上去kernel_registered_type的类型可以由variadic_kernel_fn来决定，这个逻辑是不可以直接放在构造函数里？这样开发者看着也比较直观，也避免了kernel_registered_type作为参数传进去构造

好的，我修改一下

zyfncg · 2023-02-03T03:39:12Z

paddle/phi/core/kernel_registry.h

@@ -32,7 +32,7 @@
 namespace phi {

 #define BACKEND(arg__) phi::Backend::arg__
-#define DATALAYOUT(arg__) phi::DataLayout::arg__
+#define _DATALAYOUT(arg__) phi::DataLayout::arg__


这种情况是不改op_registry.h里的宏定义名会更合适？

jiahy0825

LGTM

unify_kernel

ae98f90

YuanRisheng added 3 commits December 26, 2022 09:36

fix compile bugs

af30b2d

modify macro name

801ff91

merge develop

facb098

jiahy0825 reviewed Jan 16, 2023

View reviewed changes

Merge branch 'develop' of github.com:YuanRisheng/Paddle into unify_ke…

8d143f7

…rnel

chenwhql reviewed Jan 18, 2023

View reviewed changes

YuanRisheng added 9 commits January 18, 2023 07:55

merge develop

0276cae

Merge branch 'develop' of github.com:YuanRisheng/Paddle into unify_ke…

b614b05

…rnel

perfect code according comment

795be09

fix compile bugs

8f16fb5

fix compile bugs

8f96c26

fix ci bugs

2c1ee33

fix ci bug

33a3515

fix ci bugs

29ee1b8

fix ci bugs

1fcb651

zyfncg reviewed Feb 3, 2023

View reviewed changes

YuanRisheng added 2 commits February 3, 2023 12:04

modify code according comment

501e880

rm conv_fusion_op

ad2f95a

zyfncg approved these changes Feb 7, 2023

View reviewed changes

jiahy0825 approved these changes Feb 7, 2023

View reviewed changes

chenwhql approved these changes Feb 7, 2023

View reviewed changes

YuanRisheng merged commit e92e3aa into PaddlePaddle:develop Feb 8, 2023

qili93 mentioned this pull request Apr 26, 2024

对于Custom Device如何注册fake_quantize_range_abs_max #63888

Closed

qili93 mentioned this pull request Jun 27, 2024

[CustomDevice] Add import paddle_custom_device for custom device #65257

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PHI]Unify Fluid and PHI kernel #49328

[PHI]Unify Fluid and PHI kernel #49328

YuanRisheng commented Dec 26, 2022 •

edited

Loading

paddle-bot bot commented Dec 26, 2022

jiahy0825 Jan 16, 2023

YuanRisheng Jan 16, 2023

zyfncg Jan 18, 2023

chenwhql Jan 18, 2023

YuanRisheng Jan 18, 2023 •

edited

Loading

jiahy0825 Jan 16, 2023

YuanRisheng Jan 16, 2023

chenwhql Jan 18, 2023

YuanRisheng Jan 18, 2023

chenwhql Jan 18, 2023

chenwhql Jan 18, 2023

YuanRisheng Jan 18, 2023

zyfncg Feb 3, 2023

YuanRisheng Feb 3, 2023

chenwhql Jan 18, 2023

YuanRisheng Jan 18, 2023

chenwhql Jan 18, 2023

YuanRisheng Jan 18, 2023

zyfncg Feb 3, 2023

YuanRisheng Feb 3, 2023

zyfncg Feb 3, 2023

YuanRisheng Feb 3, 2023

zyfncg Feb 3, 2023

jiahy0825 left a comment

		@@ -241,10 +241,13 @@ class Kernel {

		bool IsValid() const { return fn_ != nullptr; }

		bool IsFuncKernel() const { return is_func_kernel_; }

[PHI]Unify Fluid and PHI kernel #49328

[PHI]Unify Fluid and PHI kernel #49328

Conversation

YuanRisheng commented Dec 26, 2022 • edited Loading

PR types

PR changes

Describe

paddle-bot bot commented Dec 26, 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

YuanRisheng Jan 18, 2023 • edited Loading

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

jiahy0825 left a comment

Choose a reason for hiding this comment

YuanRisheng commented Dec 26, 2022 •

edited

Loading

YuanRisheng Jan 18, 2023 •

edited

Loading