CARVIEW |
Navigation Menu
-
Notifications
You must be signed in to change notification settings - Fork 24.7k
Add XPU compiler version control in cmake to keep BC #139258
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
Conversation
đź”— Helpful Linksđź§Ş See artifacts and rendered test results at hud.pytorch.org/pr/139258
Note: Links to docs will display an error until the docs builds have been completed. âś… No FailuresAs of commit 5a3e292 with merge base dfcf740 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
cmake/Modules/FindSYCLToolkit.cmake
Outdated
@@ -40,6 +41,24 @@ find_file( | |||
# Due to the unrecognized compilation option `-fsycl` in other compiler. | |||
list(APPEND SYCL_INCLUDE_DIR ${SYCL_INCLUDE_SYCL_DIR}) | |||
|
|||
# Find include/sycl/version.hpp to fetch sycl compiler version | |||
if(EXISTS ${SYCL_INCLUDE_SYCL_DIR}/version.hpp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better to use find_file, to align with above lines, for example line #34
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
cmake/Modules/FindSYCLToolkit.cmake
Outdated
# 3. Strip leading and trailing spaces to get the version number. | ||
string(REGEX MATCH "__SYCL_COMPILER_VERSION[ ]+[0-9]+" TEMP1 ${SYCL_VERSION_HEADER_CONTENT}) | ||
string(REPLACE "__SYCL_COMPILER_VERSION" "" TEMP2 ${TEMP1}) | ||
string(STRIP ${TEMP2} SYCL_COMPILER_VERSION) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we give reasonable name to TEMP* vars?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@guangyey |
Currently, I'm not sure what we can do using the IGC version? |
Record for debugging as part of build log at least? |
@guangyey this looks ok. I see it modifies both Windows and Linux builds. Could you please include some testing details ? Like extract from the build log to show that this logic is performing correctly. Please rebase the PR to fix issue with aarch64 builds |
Thanks, @atalman I added and supplemented a UT in https://github.com/pytorch/pytorch/pull/139466/files#diff-90dbc2d4fedeae4036c450fc822bb422e97b4a8e576138d6811827a0c8ceeb13R433-R449 to validate the build flow as we expected. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
foreach(flag ${XPU_HOST_CXX_FLAGS}) | ||
add_definitions(${flag}) | ||
endforeach() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an anti-pattern, one should not add global flags to all targets, only to the ones that needs them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I appreciate your feedback and completely agree with your point.
Currently, we face a technical challenge with __INTEL_PREVIEW_BREAKING_CHANGES
macro in <sycl/sycl.hpp>, which is critical for maintaining ABI backward compatibility. So we have to propagate this macro to all libraries that will probably use it, such as c10_xpu, libtorch_cpu, libtorch_xpu, libtorch_python, and even third party libraries including oneDNN, torch-xpu-ops, and kineto. I will investigate this issue, and refine it once I identify a more elegant method.
The XPU CI failure depends on #140095 |
# Motivation We add a new attribute `torch.version.xpu` to facilitate the problem diagnosing and version control. # Additional Context It is aligned with `torch.version.cuda` and `torch.version.hip`. Pull Request resolved: #139466 Approved by: https://github.com/EikanWang, https://github.com/ezyang, https://github.com/atalman, https://github.com/malfet ghstack dependencies: #139258
# Motivation This PR aims to maintain backward compatibility when building PyTorch XPU with the old and new compilers. # Additional Context The details are described here. The new compiler (2025.0.0) has some breaking changes compared with the old compiler(2024.1), for examples: 1. On Windows, sycl library is named `sycl7.lib` in the old compiler but is named `sycl.lib` in the new compiler. 2. On Linux, in order to support ABI=0, we have to link `libsycl-preview.so` in the old compiler but we could link `libsycl.so` in the new compiler to have the same ABI compatibility. 3. We added a macro `SYCL_COMPILER_VERSION` to support our new code has good backward compatibility with the old compiler. Now the new feature(Event elapsed_time, memory summary, and device architecture property) introduced by the new compiler will be controlled within the macro `SYCL_COMPILER_VERSION`. Pull Request resolved: pytorch#139258 Approved by: https://github.com/EikanWang, https://github.com/atalman, https://github.com/gujinghui
# Motivation We add a new attribute `torch.version.xpu` to facilitate the problem diagnosing and version control. # Additional Context It is aligned with `torch.version.cuda` and `torch.version.hip`. Pull Request resolved: pytorch#139466 Approved by: https://github.com/EikanWang, https://github.com/ezyang, https://github.com/atalman, https://github.com/malfet ghstack dependencies: pytorch#139258
Stack from ghstack (oldest at bottom):
Motivation
This PR aims to maintain backward compatibility when building PyTorch XPU with the old and new compilers.
Additional Context
The details are described here. The new compiler (2025.0.0) has some breaking changes compared with the old compiler(2024.1), for examples:
sycl7.lib
in the old compiler but is namedsycl.lib
in the new compiler.libsycl-preview.so
in the old compiler but we could linklibsycl.so
in the new compiler to have the same ABI compatibility.SYCL_COMPILER_VERSION
to support our new code has good backward compatibility with the old compiler. Now the new feature(Event elapsed_time, memory summary, and device architecture property) introduced by the new compiler will be controlled within the macroSYCL_COMPILER_VERSION
.cc @gujinghui @PenghuiCheng @XiaobingSuper @jianyuh @jgong5 @mingfeima @sanchitintel @ashokei @jingxu10 @min-jean-cho @yanbing-j @Guobing-Chen @Xia-Weiwen @snadampal @EikanWang @fengyuan14