[PHI decoupling] migrate transpose_op.cu.h and gpu_utils.h to phi #48286
Conversation
|
你的PR提交成功，感谢你对开源项目的贡献! |
|
@YuanRisheng 可以进行 review |
| @@ -0,0 +1,189 @@ | |||
| /* Copyright (c) 2016 PaddlePaddle Authors. All Rights Reserved. | |||
paddle/phi/kernels/funcs/gpu_utils.h
Outdated
| @@ -18,11 +18,11 @@ | |||
|
|
|||
| #include <array> | |||
|
|
|||
| #include "paddle/fluid/platform/enforce.h" | |||
| #include "paddle/phi/core/enforce.h" | |||
There was a problem hiding this comment.
把gpu_utils.h这个文件放到phi/backends/gpu目录下吧
| #include "paddle/phi/backends/gpu/gpu_launch_config.h" | ||
| #include "paddle/phi/backends/gpu/gpu_primitives.h" | ||
| #include "paddle/phi/core/tensor_utils.h" | ||
| #include "paddle/phi/kernels/autotune/auto_tune_base.h" | ||
| #include "paddle/phi/kernels/funcs/gpu_utils.h" | ||
| #include "paddle/phi/kernels/funcs/transpose_op.h" |
There was a problem hiding this comment.
tanspose_op.cu.h和transpose_op.h改名为transpose_functor.cu.h/.h吧，这个原因是之前它的名字带着后缀op是由于和transpose算子绑定着，在fluid下和算子实现绑定的文件都带有_op后缀，现在transpose算子的实现逻辑已经从这俩文件里挪走了，所以改个名比较准确
| #include "paddle/phi/kernels/funcs/math_function.h" | ||
|
|
||
| namespace phi { | ||
| namespace funcs { |
There was a problem hiding this comment.
挪过来的代码，需要把原文件里的代码删掉，全局函数调用用挪过来的代码来替代，这样可以降低维护成本
There was a problem hiding this comment.
删除transpose_op.h，将对op_registry.h的依赖转移到transpose_op.cc中
There was a problem hiding this comment.
在transpose_op.cc里使用op_registry.h的头文件应该没有啥问题吧，如果其他地方用到了transpose_op.h里的op_registry.h，可以同样把registry头文件放到对应使用的地方
There was a problem hiding this comment.
@YuanRisheng 我测试了一下，好像将对op_registry.h的依赖转移到transpose_op.cc中好像有些问题，虽然在我线下的系统是能够编译过的。但是会引发线上的各种CI错误。
首先是在transpose_op.cc中引用transpose_funtor.h而不是transpose_op.h会引发codestyle CI的错误
另外删除transpose_op.h会造成许多CI错误，具体原因我还不太清楚，但是应该和依赖格式有关。
我的理解既然是fluid最终会被弃用，这里是否可以暂时留下transpose_op.h，后续再来处理呢？
目前只有transpose_op.cc这一个文件依赖transpose_op.h，后续的删除应该也是比较方便的。
There was a problem hiding this comment.
会引发codestyle CI的错误
从截图看调整下头文件顺序就可以了
另外删除transpose_op.h会造成许多CI错误
可以贴一下错误连接么，或者是哪个commit
There was a problem hiding this comment.
找一下之前的发生错误的ci链接，我看一下错误的原因，原则上我们要把以前的删掉的，因为有很多人在Paddle上开发，如果不删除的话，很有可能出现fluid下的transpose_op.h里进行了修改，但是phi下没有修改，或者是Phi下修改了，fluid对应的代码没有修改的情况，一旦代码不同步的事情发生，后续就不好维护了，因为俩份不同的代码，后来者不知道该用哪一份，还需要进行跟踪代码的修改情况
There was a problem hiding this comment.
@YuanRisheng 我在仪表盘里看了一下，好像找不到当时的CI记录了，对应的commit是format code，我又提交了一个一样的commit，看看这次是否不一样
@luotao1 调整头文件顺序是什么意思呢？我以为截图中的意思是在fluid中有类似于transpose_op.cc必须要引用同名的transpose_op.h作为头文件，否则不符合格式这样的判定。
tranpose_op.cc目前的头文件引用如下
#include "paddle/fluid/framework/op_registry.h"
#include "paddle/phi/kernels/funcs/transpose_functor.h"
#include <memory>
#include <string>
#include <vector>
#ifdef PADDLE_WITH_MKLDNN
#include "paddle/fluid/platform/mkldnn_helper.h"
#endifThere was a problem hiding this comment.
Patrick-Star125
force-pushed
the
develop
branch
from
62c4188
to
7c40def
Compare
|
@YuanRisheng 可以再 review 一下，CI 都通过了 |
|
|
||
| int num_full_tiles = framework::CeilOrFloor<int, false>( | ||
| input_long_edge, proposed_tile_long_edge); | ||
| int num_full_tiles = |
There was a problem hiding this comment.
@SigureMo 可以帮忙一起看下这个格式问题么（反馈自 @JamesLim-sy ）？是不是自动化工具的不足。
paddle/phi/kernels/funcs/transpose_functor.cu.h478行=后面加两个空格，precommit能去掉这两个空格- 但=后面加1个空格，precommit就去不得空格
正常来说，=后面应该是没有空格的
There was a problem hiding this comment.
唔，我这边测试了下，无论是一个还是两个空格，无论是直接手动 pre-commit run --files paddle/phi/kernels/funcs/transpose_functor.cu.h 触发还是直接 git commit 自动触发都是可以自动删除的呀
这里图左侧有一个空格，右侧是 commit 时自动移除了空格
…ddlePaddle#48286) * migrate transpose_op.cu.h and gpu_utils.h * format code style * fix some problems * format code * reset tranpose_op.cc * test commit * recover transpose_op.h * delete transpose_op.h * adjust header files order in transpose_op.cc
PR types
Others
PR changes
Others
Describe
因为transpose_op.cu.h依赖于gpu_utils.h，所以一起迁移过来
相关pr如下