-
Notifications
You must be signed in to change notification settings - Fork 796
[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.
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
…sources This is the technically correct thing to do, and fixes a warning with the Intel C++ compiler, as it warns when CC mode is used for linking C++ sources.
| set(install_dest_bc lib${LLVM_LIBDIR_SUFFIX}) | ||
|
|
||
| set(clang $<TARGET_FILE:clang>) | ||
| set(clangxx $<TARGET_FILE:clang> --driver-mode=g++) |
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 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++.
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.
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.
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.
Ping @bader @aelovikov-intel
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 think this falls more to the compilation/offload tools or driver than to SYCL RT.
@mdtoguchi , @AlexeySachkov , @asudarsa , what are your thoughts on this?
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 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.
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.
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.
This is the technically correct thing to do, and fixes a warning with the Intel C++ compiler, as it warns when CC mode is used for linking C++ sources.