Skip to content

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

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.

Sign up for GitHub

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

Open
chunhuanMeng wants to merge 8 commits into
base: main
Choose a base branch
from
+40 −42
Open

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
81eaa36
Remove -fsycl-host-compiler, use pure icpx compilation
chunhuanMeng Mar 5, 2026
071fc3a
Update cmake/Modules/FindSYCL/run_sycl.cmake
chunhuanMeng Mar 6, 2026
c7319f2
fix lint error
chunhuanMeng Mar 6, 2026
dd699a9
Merge branch 'main' into icpx_buid
chunhuanMeng Mar 6, 2026
88090d9
remove MSVC related build options for sycl host compiler
chunhuanMeng Mar 6, 2026
7a226e8
Update BuildFlags.cmake
chunhuanMeng Mar 6, 2026
cd86de4
Merge branch 'main' into icpx_buid
chunhuanMeng Mar 6, 2026
2cd8cdc
Update cmake/BuildFlags.cmake
chunhuanMeng Mar 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions cmake/BuildFlags.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,8 @@ macro(set_build_flags)
list(APPEND SYCL_HOST_FLAGS -fPIC)
list(APPEND SYCL_HOST_FLAGS -std=${CPP_STD})
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.
# 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.
Expand Down Expand Up @@ -188,7 +189,7 @@ macro(set_build_flags)
message(STATUS "Compile Intel GPU AOT Targets for ${AOT_TARGETS}")
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.

set(SYCL_OFFLINE_COMPILER_FLAGS "${SYCL_OFFLINE_COMPILER_AOT_OPTIONS}${SYCL_OFFLINE_COMPILER_CG_OPTIONS}")
else()
Expand Down
23 changes: 8 additions & 15 deletions cmake/Modules/FindSYCL/run_sycl.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

@guangyey guangyey Mar 6, 2026

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


set(SYCL_host_compiler_flags "-fsycl-host-compiler-options=")
set(SYCL_include_args)

foreach(dir ${SYCL_include_dirs})
# Args with spaces need quotes around them to get them to be parsed as a single argument.
if(dir MATCHES " ")
list(APPEND SYCL_include_args "-I\"${dir}\"")
string(APPEND SYCL_host_compiler_flags "-I\"${dir}\" ")
else()
list(APPEND SYCL_include_args -I${dir})
string(APPEND SYCL_host_compiler_flags "-I${dir} ")
endif()
endforeach()

Expand All @@ -66,22 +63,20 @@ endforeach()

# Adding permissive flag for MSVC build to overcome ambiguous symbol error.
if(WIN32)
string(APPEND SYCL_host_compiler_flags "/permissive- ")
list(APPEND SYCL_compile_flags "/permissive-")
endif()


list(REMOVE_DUPLICATES CMAKE_HOST_FLAGS)
foreach(flag ${CMAKE_HOST_FLAGS})
# Extra quotes are added around each flag to help SYCL parse out flags with spaces.
string(APPEND SYCL_host_compiler_flags "${flag} ")
endforeach()
foreach(def ${SYCL_compile_definitions})
string(APPEND SYCL_host_compiler_flags "-D${def} ")
# 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")
Comment on lines +71 to +75
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.
list(APPEND SYCL_compile_flags "${flag}")
Comment on lines +71 to +76
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.
endif()
endforeach()

# string(APPEND SYCL_host_compiler_flags "\"")
set(SYCL_host_compiler "-fsycl-host-compiler=${SYCL_host_compiler}")

# SYCL_execute_process - Executes a command with optional command echo and status message.
#
# status - Status message to print if verbose is true
Expand Down Expand Up @@ -140,8 +135,6 @@ SYCL_execute_process(
"${source_file}"
-o "${generated_file}"
${SYCL_include_args}
${SYCL_host_compiler}
${SYCL_host_compiler_flags}
${SYCL_compile_flags}
)

Expand Down
13 changes: 13 additions & 0 deletions cmake/Modules/FindSYCLToolkit.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,3 +259,16 @@ set(__INTEL_LLVM_COMPILER "${__INTEL_LLVM_COMPILER}" CACHE STRING "Intel llvm co

message(DEBUG "The SYCL compiler is ${SYCL_COMPILER}")
message(DEBUG "The SYCL Flags are ${SYCL_FLAGS}")

# 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
)
Comment on lines +213 to +221
Copy link
Contributor

@CuiYifeng CuiYifeng Mar 6, 2026

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?

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

@guangyey guangyey Mar 6, 2026

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.

Comment on lines +222 to +224
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.
Loading