Skip to content

[SYCL][libdevice]: Use g++ driver mode (instead of CC mode) for sycl sources #15885

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

Closed
wants to merge 1 commit into from
Closed

[SYCL][libdevice]: Use g++ driver mode (instead of CC mode) for sycl sources #15885

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 4 additions & 4 deletions libdevice/cmake/modules/SYCLLibdevice.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ set(install_dest_obj lib${LLVM_LIBDIR_SUFFIX})
set(install_dest_obj-new-offload lib${LLVM_LIBDIR_SUFFIX})
set(install_dest_bc lib${LLVM_LIBDIR_SUFFIX})

set(clang $<TARGET_FILE:clang>)
set(clangxx $<TARGET_FILE:clang> --driver-mode=g++)
Copy link
Contributor

@bader bader Oct 28, 2024

Choose a reason for hiding this comment

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

I think the right place to add compiler options is to extend compile_opts variable. If I understand the problem correctly, the option we need to set is rather -x c++.

Copy link
Contributor Author

@Maetveis Maetveis Oct 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If I understand the problem correctly, the option we need to set is rather -x c++

That's not case, -x c++ is already implicitly set based on the file extension. ICX is specifically looking for the name it is invoked as (argv[0]), which set it's driver mode.

Since clang++ is not a full CMake target (but a symlink) we can't do $<TARGET_FILE:clang++>, but the "driver mode" can also be set explicitly via the option as I did here. This is also why I included the option as part of the variable for the the tool "executable"; In my thinking we're invoking clang++, but can't spell it exactly as such.

I don't mind moving the option to compile_opts though if that's preferred.

Copy link
Contributor Author

@Maetveis Maetveis Nov 8, 2024

Choose a reason for hiding this comment

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

Ping @bader @aelovikov-intel

Copy link
Contributor

@aelovikov-intel aelovikov-intel Nov 8, 2024

Choose a reason for hiding this comment

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

I think this falls more to the compilation/offload tools or driver than to SYCL RT.

@mdtoguchi , @AlexeySachkov , @asudarsa , what are your thoughts on this?

Copy link
Contributor Author

@Maetveis Maetveis Nov 8, 2024

Choose a reason for hiding this comment

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

I somewhat agree and in-fact my initial reaction was fixing icx to not issue this warning for this case. We're passing it -fsycl-device so the host c++ runtime libs are irrelevant. I will revisit that patch, but I believe its not completely wrong to suggest that clang++ be used for C++ sources, so we might apply this too.

Copy link
Contributor

@mdtoguchi mdtoguchi Nov 8, 2024

Choose a reason for hiding this comment

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

The use of clang --driver-mode=g++ does tell the driver to work as the C++ compiler but from what I understand, the diagnostic that is emitted is happening when we are using -fsycl-device-only. The diagnostic is a link specific diagnostic and since we aren't really doing a host link step with -fsycl-device-only we can probably address the diagnostic emission with a modification to the compiler driver to be a bit smarter when emitting this diagnostic.

set(llvm-ar $<TARGET_FILE:llvm-ar>)
set(llvm-link $<TARGET_FILE:llvm-link>)
set(llvm-opt $<TARGET_FILE:opt>)
Expand Down Expand Up @@ -118,7 +118,7 @@ function(compile_lib filename)

add_custom_command(
OUTPUT ${devicelib-file}
COMMAND ${clang} ${compile_opts} ${ARG_EXTRA_OPTS}
COMMAND ${clangxx} ${compile_opts} ${ARG_EXTRA_OPTS}
${CMAKE_CURRENT_SOURCE_DIR}/${ARG_SRC} -o ${devicelib-file}
MAIN_DEPENDENCY ${ARG_SRC}
DEPENDS ${ARG_DEPENDENCIES}
Expand Down Expand Up @@ -418,7 +418,7 @@ function(add_lib_imf name)

add_custom_command(
OUTPUT ${ARG_DIR}/${name}.${${ARG_FTYPE}-suffix}
COMMAND ${clang} ${compile_opts} ${ARG_EXTRA_OPTS}
COMMAND ${clangxx} ${compile_opts} ${ARG_EXTRA_OPTS}
-I ${CMAKE_CURRENT_SOURCE_DIR}/imf
${imf_${ARG_DTYPE}_fallback_src}
-o
Expand Down Expand Up @@ -518,7 +518,7 @@ foreach(dtype IN ITEMS bf16 fp32 fp64)
endif()
add_custom_command(
OUTPUT ${${ftype}_binary_dir}/imf-${dtype}-host.${${ftype}-suffix}
COMMAND ${clang} ${${ftype}_host_compile_opts}
COMMAND ${clangxx} ${${ftype}_host_compile_opts}
${CMAKE_CURRENT_SOURCE_DIR}/${wrapper_name}
-o ${${ftype}_binary_dir}/imf-${dtype}-host.${${ftype}-suffix}
MAIN_DEPENDENCY ${CMAKE_CURRENT_SOURCE_DIR}/${wrapper_name}
Expand Down
Loading