Skip to content

WIP Remove -fsycl-host-compiler, use pure icpx compilation#2994

Open
chunhuanMeng wants to merge 8 commits intomainfrom
icpx_buid
Open

WIP Remove -fsycl-host-compiler, use pure icpx compilation#2994
chunhuanMeng wants to merge 8 commits intomainfrom
icpx_buid

Conversation

@chunhuanMeng
Copy link
Contributor

@chunhuanMeng chunhuanMeng commented Mar 5, 2026

This pull request refactors SYCL build flag handling and improves compatibility with Intel compilers and non-icpx linkers. It removes legacy host compiler flag logic, streamlines flag propagation, and ensures required Intel runtime libraries are linked when using alternative linkers. The changes focus on modernizing the build system for SYCL and addressing issues with symbol mismatches and compiler breaking changes.

Build flag handling and propagation:

  • Removed legacy MSVC and GNU-specific host flag logic from cmake/BuildFlags.cmake, unified flag handling, and added support for Intel compiler breaking changes via -D__INTEL_PREVIEW_BREAKING_CHANGES. [1] [2]
  • Ensured SYCL_HOST_FLAGS are correctly included in SYCL_COMPILE_FLAGS for consistent flag propagation during compilation.

SYCL host compiler flag refactor:

  • Eliminated string-based host compiler flag construction in run_sycl.cmake, switching to list-based flag management for improved clarity and maintainability. [1] [2]
  • Updated the SYCL compilation command to use the new flag list, removing references to the old host compiler and flag variables.

Intel runtime library linking:

  • Added logic in FindSYCLToolkit.cmake to locate and append the Intel runtime library (libintlc) when linking SYCL objects with non-icpx linkers, preventing undefined symbol errors.

Copilot AI review requested due to automatic review settings March 5, 2026 08:42
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This pull request updates the project’s SYCL CMake integration to stop using -fsycl-host-compiler* and instead drive SYCL compilation via icpx/icx directly, while also improving propagation of host/definition flags and addressing Intel toolchain linking needs.

Changes:

  • Add Intel preview-breaking-changes define to SYCL host flags and ensure host flags are included in SYCL_COMPILE_FLAGS.
  • Simplify run_sycl.cmake by removing host-compiler option string construction and passing relevant -D defines directly to the SYCL compiler.
  • Add discovery/linking of Intel runtime library libintlc via FindSYCLToolkit.cmake.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
cmake/Modules/FindSYCLToolkit.cmake Attempts to auto-link libintlc to avoid missing Intel runtime symbols when using non-icpx linkers.
cmake/Modules/FindSYCL/run_sycl.cmake Removes -fsycl-host-compiler* usage; forwards compile definitions/selected host defines directly to icpx/icx.
cmake/BuildFlags.cmake Adds Intel preview define; ensures SYCL_HOST_FLAGS propagate into SYCL_COMPILE_FLAGS.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

endif()

set(SYCL_COMPILE_FLAGS ${SYCL_COMPILE_FLAGS} ${SYCL_KERNEL_OPTIONS})
set(SYCL_COMPILE_FLAGS ${SYCL_COMPILE_FLAGS} ${SYCL_HOST_FLAGS} ${SYCL_KERNEL_OPTIONS})
Copy link

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

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

SYCL_HOST_FLAGS now gets appended directly into SYCL_COMPILE_FLAGS. On Windows, SYCL_HOST_FLAGS is populated with MSVC-style options (e.g. /std:..., /MD, /EHsc), but SYCL_EXECUTABLE is set to icx (not icx-cl). If icx is running in clang-style mode, these /... flags may be rejected or misinterpreted. Consider either translating the MSVC flags to clang equivalents when compiling via icx, or switching the SYCL compiler executable on Windows to an MSVC-compatible driver (e.g. icx-cl) when passing MSVC flags.

Suggested change
set(SYCL_COMPILE_FLAGS ${SYCL_COMPILE_FLAGS} ${SYCL_HOST_FLAGS} ${SYCL_KERNEL_OPTIONS})
if(WIN32)
# On Windows, SYCL_HOST_FLAGS may contain MSVC-style flags (/std:..., /MD, /EHsc).
# Only pass these flags when the SYCL compiler is a CL-style driver that understands them.
# Always add kernel options.
set(SYCL_COMPILE_FLAGS ${SYCL_COMPILE_FLAGS} ${SYCL_KERNEL_OPTIONS})
if(SYCL_COMPILER MATCHES "icx-cl(\\.exe)?"
OR SYCL_COMPILER MATCHES "dpcpp-cl(\\.exe)?"
OR SYCL_COMPILER MATCHES "cl(\\.exe)?")
# CL-style drivers: safe to add MSVC-style host flags.
set(SYCL_COMPILE_FLAGS ${SYCL_HOST_FLAGS} ${SYCL_COMPILE_FLAGS})
endif()
else()
# Non-Windows: keep existing behavior.
set(SYCL_COMPILE_FLAGS ${SYCL_COMPILE_FLAGS} ${SYCL_HOST_FLAGS} ${SYCL_KERNEL_OPTIONS})
endif()

Copilot uses AI. Check for mistakes.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings March 6, 2026 05:31
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

@@ -41,17 +41,14 @@ set(SYCL_compile_definitions [==[@SYCL_compile_definitions@]==]) # list

list(REMOVE_DUPLICATES SYCL_INCLUDE_DIRS)
Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

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

list(REMOVE_DUPLICATES SYCL_INCLUDE_DIRS) references a variable that is never set in this script. The include list variable used everywhere else is SYCL_include_dirs, so this call currently has no effect (and duplicates in the include args won’t be removed).

Suggested change
list(REMOVE_DUPLICATES SYCL_INCLUDE_DIRS)
list(REMOVE_DUPLICATES SYCL_include_dirs)

Copilot uses AI. Check for mistakes.
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is unrelated to this PR, but please rename it to SYCL_include_dirs

Comment on lines +71 to +75
# Extract -D defines from CMAKE_HOST_FLAGS and pass them directly to icpx,
# since host compiler is removed. This is needed for macros like
# HAVE_AVX512_CPU_DEFINITION that control signatures, to avoid
# symbol mismatch with libtorch_cpu.so.
if(flag MATCHES "^-D")
Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

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

After removing -fsycl-host-compiler* usage, the SYCL_host_compiler variable set near the top of this script is no longer referenced anywhere. Consider removing it from the generated script/template to avoid implying that a separate host compiler is still used.

Copilot uses AI. Check for mistakes.
Comment on lines +71 to +76
# Extract -D defines from CMAKE_HOST_FLAGS and pass them directly to icpx,
# since host compiler is removed. This is needed for macros like
# HAVE_AVX512_CPU_DEFINITION that control signatures, to avoid
# symbol mismatch with libtorch_cpu.so.
if(flag MATCHES "^-D")
list(APPEND SYCL_compile_flags "${flag}")
Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

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

CMAKE_HOST_FLAGS can contain preprocessor defines in MSVC form (/D...) on Windows, but this loop only forwards flags that match ^-D. That means important defines from CMAKE_CXX_FLAGS may be silently dropped on WIN32 builds. Consider also matching /D (and forwarding it in the appropriate form for icx).

Suggested change
# Extract -D defines from CMAKE_HOST_FLAGS and pass them directly to icpx,
# since host compiler is removed. This is needed for macros like
# HAVE_AVX512_CPU_DEFINITION that control signatures, to avoid
# symbol mismatch with libtorch_cpu.so.
if(flag MATCHES "^-D")
list(APPEND SYCL_compile_flags "${flag}")
# Extract -D (GCC/Clang) or /D (MSVC) defines from CMAKE_HOST_FLAGS and pass
# them directly to the SYCL compiler, since the host compiler is removed.
# This is needed for macros like HAVE_AVX512_CPU_DEFINITION that control
# signatures, to avoid symbol mismatch with libtorch_cpu.so.
if(flag MATCHES "^-D" OR flag MATCHES "^/D")
if(flag MATCHES "^/D")
string(REGEX REPLACE "^/D" "-D" converted_flag "${flag}")
else()
set(converted_flag "${flag}")
endif()
list(APPEND SYCL_compile_flags "${converted_flag}")

Copilot uses AI. Check for mistakes.
Comment on lines 59 to 62
list(APPEND SYCL_HOST_FLAGS -Wunused-variable)
list(APPEND SYCL_HOST_FLAGS -Wno-interference-size)
# Required for Intel compiler breaking changes;
list(APPEND SYCL_HOST_FLAGS -D__INTEL_PREVIEW_BREAKING_CHANGES)
# Some versions of DPC++ compiler pass paths to SYCL headers as user include paths (`-I`) rather
Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

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

The PR description says -D__INTEL_PREVIEW_BREAKING_CHANGES is added to SYCL_HOST_FLAGS, but the current change adds it only in the GNU branch. If Windows/MSVC builds also use icx/SYCL compilation, they may need the same define (or the PR description should be updated to clarify it’s Linux-only).

Copilot uses AI. Check for mistakes.
@chunhuanMeng chunhuanMeng changed the title Remove -fsycl-host-compiler, use pure icpx compilation WIP Remove -fsycl-host-compiler, use pure icpx compilation Mar 6, 2026
Copilot AI review requested due to automatic review settings March 6, 2026 06:21
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

)
if(SYCL_INTLC_LIBRARY)
list(APPEND SYCL_LIBRARY ${SYCL_INTLC_LIBRARY})
endif()
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting! I think we could upstream this component to https://github.com/pytorch/pytorch/blob/main/cmake/Modules/FindSYCLToolkit.cmake
I intend to remove the file cmake/Modules/FindSYCLToolkit.cmake from torch-xpu-ops later.

Copilot AI review requested due to automatic review settings March 6, 2026 09:10
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Comment on lines +49 to +55
list(APPEND SYCL_HOST_FLAGS -fPIC)
list(APPEND SYCL_HOST_FLAGS -std=${CPP_STD})
list(APPEND SYCL_HOST_FLAGS -Wunused-variable)
# Required for Intel compiler breaking changes;
list(APPEND SYCL_HOST_FLAGS -D__INTEL_PREVIEW_BREAKING_CHANGES)
# Some versions of DPC++ compiler pass paths to SYCL headers as user include paths (`-I`) rather
# than system paths (`-isystem`). This makes host compiler to report warnings encountered in the
Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

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

SYCL_HOST_FLAGS is now populated with GCC/Clang-style options (-fPIC, -std=..., -Wunused-variable, -D...) even when CMAKE_CXX_COMPILER_ID is MSVC (the surrounding if() explicitly allows MSVC). These flags are not compatible with MSVC/clang-cl style and will break Windows builds (and also affects TORCH_XPU_OPS_FLAGS which is set from SYCL_HOST_FLAGS). Consider restoring the compiler-specific branches and using MSVC equivalents (e.g. /std:, /MD, /EHsc, /D...) while keeping the Intel macro define only where supported.

Copilot uses AI. Check for mistakes.
# the issue we wrap paths to SYCL headers in `-isystem`.
if(SYCL_COMPILER_VERSION VERSION_LESS 20250300)
foreach(FLAGS IN LISTS SYCL_INCLUDE_DIR)
list(APPEND SYCL_HOST_FLAGS "-isystem ${FLAGS}")
Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

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

list(APPEND SYCL_HOST_FLAGS "-isystem ${FLAGS}") appends -isystem and the path as a single list element. Since SYCL_HOST_FLAGS are now appended into SYCL_COMPILE_FLAGS and passed as argv elements, this will be forwarded to the compiler as one argument containing a space (e.g. "-isystem /path"), which clang/icpx typically rejects. Append -isystem and the include path as separate list elements (or -isystem plus a quoted path) so it becomes two argv tokens.

Suggested change
list(APPEND SYCL_HOST_FLAGS "-isystem ${FLAGS}")
list(APPEND SYCL_HOST_FLAGS -isystem "${FLAGS}")

Copilot uses AI. Check for mistakes.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings March 6, 2026 10:50
@chunhuanMeng chunhuanMeng added windows_ci Only for Windows CI trigger labels Mar 6, 2026
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Comment on lines +49 to 67
list(APPEND SYCL_HOST_FLAGS -fPIC)
list(APPEND SYCL_HOST_FLAGS -std=${CPP_STD})
list(APPEND SYCL_HOST_FLAGS -Wunused-variable)
# Required for Intel compiler breaking changes;
list(APPEND SYCL_HOST_FLAGS -D__INTEL_PREVIEW_BREAKING_CHANGES)
# Some versions of DPC++ compiler pass paths to SYCL headers as user include paths (`-I`) rather
# than system paths (`-isystem`). This makes host compiler to report warnings encountered in the
# SYCL headers, such as deprecated warnings, even if warned API is not actually used in the program.
# We expect that this issue will be addressed in the later version of DPC++ compiler. To workaround
# the issue we wrap paths to SYCL headers in `-isystem`.
if(SYCL_COMPILER_VERSION VERSION_LESS 20250300)
foreach(FLAGS IN LISTS SYCL_INCLUDE_DIR)
list(APPEND SYCL_HOST_FLAGS "-isystem ${FLAGS}")
endforeach()
endif()
# Excluding warnings which flood the compilation output
# TODO: fix warnings in the source code and then reenable them in compilation
list(APPEND SYCL_HOST_FLAGS -Wno-sign-compare)

Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

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

SYCL_HOST_FLAGS is later propagated to TORCH_XPU_OPS_FLAGS (and used via target_compile_options(...)), so it must be valid for the host compiler (GCC/MSVC). After this change, GCC/Clang-style flags like -fPIC, -std=..., and -D... will be passed to MSVC builds (where they are invalid; MSVC expects e.g. /std: and /D). Please reintroduce compiler-id-specific host flags (or split the concept into separate HOST_CXX_FLAGS vs SYCL_COMPILER_FLAGS so icpx-only flags don't leak into MSVC compilation).

Suggested change
list(APPEND SYCL_HOST_FLAGS -fPIC)
list(APPEND SYCL_HOST_FLAGS -std=${CPP_STD})
list(APPEND SYCL_HOST_FLAGS -Wunused-variable)
# Required for Intel compiler breaking changes;
list(APPEND SYCL_HOST_FLAGS -D__INTEL_PREVIEW_BREAKING_CHANGES)
# Some versions of DPC++ compiler pass paths to SYCL headers as user include paths (`-I`) rather
# than system paths (`-isystem`). This makes host compiler to report warnings encountered in the
# SYCL headers, such as deprecated warnings, even if warned API is not actually used in the program.
# We expect that this issue will be addressed in the later version of DPC++ compiler. To workaround
# the issue we wrap paths to SYCL headers in `-isystem`.
if(SYCL_COMPILER_VERSION VERSION_LESS 20250300)
foreach(FLAGS IN LISTS SYCL_INCLUDE_DIR)
list(APPEND SYCL_HOST_FLAGS "-isystem ${FLAGS}")
endforeach()
endif()
# Excluding warnings which flood the compilation output
# TODO: fix warnings in the source code and then reenable them in compilation
list(APPEND SYCL_HOST_FLAGS -Wno-sign-compare)
if(CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
list(APPEND SYCL_HOST_FLAGS -fPIC)
list(APPEND SYCL_HOST_FLAGS -std=${CPP_STD})
list(APPEND SYCL_HOST_FLAGS -Wunused-variable)
# Required for Intel compiler breaking changes;
list(APPEND SYCL_HOST_FLAGS -D__INTEL_PREVIEW_BREAKING_CHANGES)
# Some versions of DPC++ compiler pass paths to SYCL headers as user include paths (`-I`) rather
# than system paths (`-isystem`). This makes host compiler to report warnings encountered in the
# SYCL headers, such as deprecated warnings, even if warned API is not actually used in the program.
# We expect that this issue will be addressed in the later version of DPC++ compiler. To workaround
# the issue we wrap paths to SYCL headers in `-isystem`.
if(SYCL_COMPILER_VERSION VERSION_LESS 20250300)
foreach(FLAGS IN LISTS SYCL_INCLUDE_DIR)
list(APPEND SYCL_HOST_FLAGS "-isystem ${FLAGS}")
endforeach()
endif()
# Excluding warnings which flood the compilation output
# TODO: fix warnings in the source code and then reenable them in compilation
list(APPEND SYCL_HOST_FLAGS -Wno-sign-compare)
elseif(CMAKE_CXX_COMPILER_ID STREQUAL "MSVC")
# Map requested C++ standard to MSVC's /std: flag.
if(REPLACE_FLAGS_FOR_SYCLTLA)
set(_SYCL_HOST_STD_FLAG "/std:c++20")
else()
set(_SYCL_HOST_STD_FLAG "/std:c++17")
endif()
list(APPEND SYCL_HOST_FLAGS ${_SYCL_HOST_STD_FLAG})
# Required for Intel compiler breaking changes;
list(APPEND SYCL_HOST_FLAGS "/D__INTEL_PREVIEW_BREAKING_CHANGES")
# MSVC-specific warning control can be added here if needed. We intentionally
# avoid passing GCC/Clang-style warning flags to MSVC.
endif()

Copilot uses AI. Check for mistakes.
Comment on lines +222 to +224
if(SYCL_INTLC_LIBRARY)
list(APPEND SYCL_LIBRARY ${SYCL_INTLC_LIBRARY})
endif()
Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

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

The comment says libintlc is required when linking SYCL objects with a non-icpx linker, but the code appends it to SYCL_LIBRARY unconditionally whenever it is found. If this is truly only needed for non-icpx linkers, please gate the append on the actual linker/toolchain being used; otherwise update the comment to reflect that SYCL_LIBRARY will always include intlc when available.

Copilot uses AI. Check for mistakes.
Comment on lines +213 to +221
# Find Intel runtime library (libintlc), required when linking SYCL objects
# with a non-icpx linker (e.g. g++). icpx adds this automatically, but g++
# does not, leading to undefined reference to '_intel_fast_memcpy' etc.
find_library(
SYCL_INTLC_LIBRARY
NAMES intlc
HINTS ${SYCL_LIBRARY_DIR}
NO_DEFAULT_PATH
)
Copy link
Contributor

Choose a reason for hiding this comment

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

@chunhuanMeng As we discussed, could you check original status of intlc?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

windows_ci Only for Windows CI trigger

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants