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 decoupling] move softmax from fluid to phi and remove cpu_vec.h in fluid #48970

Merged
merged 16 commits into from
Dec 15, 2022

Conversation

huangjiyi
Copy link
Member

@huangjiyi huangjiyi commented Dec 9, 2022

PR types

Others

PR changes

Others

Describe

  • [phi] Decoupled phi from fluid tracking issue #47615
  • softmax.h, softmax_impl.h, softmax.cc, softmax.cupaddle/fluid/operators/math/ 目录移动到 paddle/phi/kernels/funcs/ 目录。
  • cpu_info 完全移动到 phi
  • 移除 paddle/fluid/operators/math/cpu_vec.h,因为 paddle/phi/kernels/funcs/cpu_vec.h 有相同的实现

@paddle-bot
Copy link

paddle-bot bot commented Dec 9, 2022

你的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.

@paddle-bot paddle-bot bot added contributor External developers status: proposed labels Dec 9, 2022
Comment on lines 19 to 21
#include "paddle/phi/core/dense_tensor.h"
#include "paddle/phi/kernels/funcs/eigen/extensions.h"
#include "unsupported/Eigen/CXX11/Tensor"
Copy link
Member Author

@huangjiyi huangjiyi Dec 9, 2022

Choose a reason for hiding this comment

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

这个改动是因为将 softmax_impl.h 中的 platform::EigenMatrix 替换为 phi::EigenMatrix 后,不引入 paddle/phi/kernels/funcs/eigen/extensions.h 的话编译会报下面这个错误,报错的代码好像是在第三方 Eigen 库中,这个错误是说有多个匹配的重载函数,但我不知道怎么处理。后来通过对比 fluid eigenphi eigen 两个头文件的内容尝试在 phi eigen 头文件中包含 eigen/extensions.h 就不报错了,虽然不知道是什么原因。
image

Comment on lines +39 to +51
#ifndef PADDLE_WITH_XBYAK
#ifdef _WIN32
#define cpuid(reg, x) __cpuidex(reg, x, 0)
#else
#if !defined(WITH_NV_JETSON) && !defined(PADDLE_WITH_ARM) && \
!defined(PADDLE_WITH_SW) && !defined(PADDLE_WITH_MIPS)
#include <cpuid.h>
inline void cpuid(int reg[4], int x) {
__cpuid_count(x, 0, reg[0], reg[1], reg[2], reg[3]);
}
#endif
#endif
#endif
Copy link
Member Author

@huangjiyi huangjiyi Dec 10, 2022

Choose a reason for hiding this comment

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

CI-Windows-OPENBLAS 里有一个关于这段代码的报错我处理不了了

2022-12-10 11:56:16 FAILED: paddle/phi/backends/CMakeFiles/phi_backends.dir/cpu/cpu_info.cc.obj 
...
2022-12-10 11:56:16 ..\paddle\phi\backends\cpu\cpu_info.cc(71): error C3861: '__cpuidex': identifier not found
2022-12-10 11:56:16 ..\paddle\phi\backends\cpu\cpu_info.cc(75): error C3861: '__cpuidex': identifier not found
2022-12-10 11:56:16 ..\paddle\phi\backends\cpu\cpu_info.cc(84): error C3861: '__cpuidex': identifier not found
2022-12-10 11:56:18 ..\paddle\phi\backends\cpu\cpu_info.cc(102): error C3861: '__cpuidex': identifier not found

phi 下的 cpu_info.cc 是我这次 PR 新加的,报错说找不到 __cpuidex 这个函数的声明,但在 cpu_info.h 中有包含相应的头文件:
image

Copy link
Member Author

Choose a reason for hiding this comment

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

@YuanRisheng 帮忙看一下这个问题

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

Choose a reason for hiding this comment

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

代码没看出啥问题,我先rerun下看看情况

Copy link
Member Author

@huangjiyi huangjiyi Dec 12, 2022

Choose a reason for hiding this comment

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

还是失败了,会不会是因为没有找到宏 __AVX2____AVX__ 导致没有包含目标头文件,但是这段代码在原来 fluid 下却没问题,所以搞不懂。
image

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

Choose a reason for hiding this comment

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

这个我本地解决了,目前不太清楚具体原因,但从经验角度考虑,问题的原因很可能是cmake依赖关系混乱出现的问题,Fluid下cpu_info被好几个地方依赖,cpu_info又依赖phi_backends,phi_backends本身又被好几个地方依赖,感觉很乱。我本地把Fluid目录下的cpu_info.cc全部挪到PHI下后,删了Fluid的cmakelist里的cpu_info编译target,把Fluid依赖cpu_info的地方替换成phi_backends后,本地编译可以通过,你可以试试。

Copy link
Member Author

@huangjiyi huangjiyi Dec 13, 2022

Choose a reason for hiding this comment

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

这个我本地解决了,目前不太清楚具体原因,但从经验角度考虑,问题的原因很可能是cmake依赖关系混乱出现的问题,Fluid下cpu_info被好几个地方依赖,cpu_info又依赖phi_backends,phi_backends本身又被好几个地方依赖,感觉很乱。我本地把Fluid目录下的cpu_info.cc全部挪到PHI下后,删了Fluid的cmakelist里的cpu_info编译target,把Fluid依赖cpu_info的地方替换成phi_backends后,本地编译可以通过,你可以试试。

好的,我试一下,谢谢老师

@huangjiyi
Copy link
Member Author

@YuanRisheng ,帮忙 Review 一下

@@ -9,7 +9,7 @@ file(APPEND ${jit_file} "\#include \"paddle/fluid/operators/jit/helper.h\"\n")
file(APPEND ${jit_file}
"\#include \"paddle/fluid/operators/jit/registry.h\"\n\n")

set(JIT_KERNEL_DEPS cpu_info cblas gflags enforce place xxhash)
set(JIT_KERNEL_DEPS device_context cblas gflags enforce place xxhash)
Copy link
Contributor

Choose a reason for hiding this comment

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

这里device_context能否改成phi_backends?

Copy link
Member Author

@huangjiyi huangjiyi Dec 14, 2022

Choose a reason for hiding this comment

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

@YuanRisheng 这里我最开始用的是 phi_backends,然后编译报了找不到 device_context 中一些定义的错误,然后我就改成了 device_context(device_context 也依赖了 phi_backends)

#endif
#endif
#endif

#include "paddle/phi/backends/cpu/cpu_info.h"

namespace paddle {
Copy link
Contributor

Choose a reason for hiding this comment

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

直接把cpu_info.h也删了吧,改的文件应该也不会增加太多

Copy link
Member Author

Choose a reason for hiding this comment

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

已处理

@luotao1
Copy link
Contributor

luotao1 commented Dec 15, 2022

@YuanRisheng 可以再review一下

@huangjiyi
Copy link
Member Author

@luotao1 ,CI-OP-benchmark 超时取消需要怎么处理,昨天也是这个情况 Rerun 后还是一样的结果

@luotao1
Copy link
Contributor

luotao1 commented Dec 15, 2022

CI-OP-benchmark 超时取消需要怎么处理,昨天也是这个情况 Rerun 后还是一样的结果

Done 已豁免

@huangjiyi
Copy link
Member Author

@XiaoguangHu01, Request approval for change 20+ files

Copy link
Contributor

@XiaoguangHu01 XiaoguangHu01 left a comment

Choose a reason for hiding this comment

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

LGTM

@huangjiyi
Copy link
Member Author

@luotao1,帮忙 Merge 一下

@luotao1 luotao1 merged commit 344b99e into PaddlePaddle:develop Dec 15, 2022
@huangjiyi huangjiyi deleted the decouple_softmax_h branch March 8, 2023 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contributor External developers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants