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

[dipu] partially refactor CMakeLists.txt and tidy almost all source code. #598

Merged
merged 25 commits into from
Jan 12, 2024

Conversation

wiryls
Copy link
Collaborator

@wiryls wiryls commented Jan 4, 2024

理应拆分为俩仨个 PR,但最近集群环境更新,需要重新搭建开发环境。因此可能不方便 debug,于是打包提交这个 PR。

这次的 PR 内容比较多,主要包含四类事项:

  1. 为 CI 新增 clang-tidy 的流程
  2. 重构一个 CMakeLists.txt 文件,关于 CMakeLists.txt 改进问题 #479之后还需要
    • 重新组织当前 CMakeLists.txt
    • 重构 vendor 的 CMakeLists.txt
    • 重构顶层 CMakeLists.txt
  3. 简单的代码 tidy
    • 调整头文件格式("<
    • 移除少量无用的头文件(危险的操作
    • 根据 tidy 的提示进行代码更新
  4. 简单的代码逻辑重构
    • dipu/torch_dipu/csrc_dipu/aten/RegisterDIPU.cpp

还有一些问题:

  • csrc_dipu/CMakeLists.txt 是否与旧版功能上等价?
  • csrc_dipu/runtime/distributed/c10dOps.cpp 是否必须使用 const_cast
  • csrc_dipu/runtime/distributed/c10dOps.cpp 是否可以可以将 barrier_dipu 参数 tensor 修改为 const & 经过测试不可以随便改,需要与 Python 层保持接口一致

以及一些经验:

  • 在修改函数参数类型前,需要确定它不是接口层,也不用于 pybind 等
  • 当前代码没有区分公私有头文件,关系也较为混乱,因此移除头文件变得比较危险
    • 移除某些头文件之后编译时没有问题,但运行时出现异常
    • 最好翻翻有没有什么好用的分析工具

@wiryls wiryls added the DIPU DIPU related label Jan 4, 2024
@wiryls wiryls changed the title [DIPU] 部分重构 CMakeLists.txt 同时修改了 dipu 绝大多数需要 tidy 的文件 [DIPU] partially refactor CMakeLists.txt and tidy almost source code. Jan 4, 2024
set(GENERATED_KERNELS "${CMAKE_CURRENT_SOURCE_DIR}/aten/ops/AutoGenedKernels.cpp")
set(GENERATED_KERNELS_SCRIPT "${PROJECT_SOURCE_DIR}/scripts/autogen_diopi_wrapper/autogen_diopi_wrapper.py")
set(GENERATED_KERNELS_CONFIG "${PROJECT_SOURCE_DIR}/scripts/autogen_diopi_wrapper/diopi_functions.yaml")
set(GENERATED_KERNELS_VENDOR "${PROJECT_SOURCE_DIR}/third_party/DIOPI/impl/${UsedVendor}/convert_config.yaml")
Copy link
Collaborator Author

@wiryls wiryls Jan 4, 2024

Choose a reason for hiding this comment

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

目前这里的 config 采用了旧版逻辑。但我们还提供了使用使用外部 DIOPI 的配置,这种实现并不兼容外部 DIOPI 的情况。

考虑到目前只有 s2 使用外部 DIOPI,并且只有 camb 在用 convert_config.yaml,因此暂时没有问题。关于后续修复方法,可以考虑从 targets 绑定的变量获取 convert_config.yaml 路径。

或者再确认一下是否还有使用外部 DIOPI 的需求,能移除最好,毕竟维护起来挺麻烦的,容易像这里一样遗漏。

file(GLOB BASE_FILES base/*.cpp)
file(GLOB UTILS_FILES utils/*.cpp)
file(GLOB DIOPI_RT_FILES diopirt/*.cpp)
file(GLOB PROFILER_FILES profiler/*.cpp)
Copy link
Collaborator Author

@wiryls wiryls Jan 4, 2024

Choose a reason for hiding this comment

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

We do not recommend using GLOB to collect a list of source files from your source tree. —— CMake: file

另外,这样修改之后,每次新增 cpp 文件都需要修改此处的 CMakeLists.txt 了。

@wiryls wiryls changed the title [DIPU] partially refactor CMakeLists.txt and tidy almost source code. [dipu] partially refactor CMakeLists.txt and tidy almost all source code. Jan 4, 2024
@wiryls wiryls marked this pull request as ready for review January 5, 2024 11:45
@wiryls wiryls requested a review from fandaoyi January 5, 2024 11:46
@wiryls wiryls mentioned this pull request Jan 5, 2024
7 tasks
@wiryls wiryls added the refactor just better coding label Jan 10, 2024
@wiryls wiryls requested a review from wugeshui as a code owner January 11, 2024 07:03
@wiryls wiryls marked this pull request as draft January 11, 2024 07:10
@wiryls wiryls marked this pull request as ready for review January 11, 2024 07:10
@wiryls wiryls marked this pull request as draft January 11, 2024 09:00
@wiryls wiryls marked this pull request as ready for review January 11, 2024 12:51
@wiryls wiryls added the enhancement New feature or request label Jan 12, 2024
@mrdanielw mrdanielw merged commit b77713f into DeepLink-org:main Jan 12, 2024
23 of 24 checks passed
@wiryls wiryls deleted the refactor/w/tidy-and-cmake branch January 12, 2024 11:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DIPU DIPU related enhancement New feature or request refactor just better coding
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants