SYCL_EXTERNAL definition is inconsistent.

[bader]

Describe the bug

Here is an example of the code to demonstrate the inconsistency:

$ cat example.cpp
SYCL_EXTERNAL int mul(int x) {
  return x * 42;
}
$ clang++ -fsycl-device-only example.cpp
$ clang++ -fsycl-host-only example.cpp
example.cpp:1:1: error: unknown type name 'SYCL_EXTERNAL'
    1 | SYCL_EXTERNAL int mul(int x) {
      | ^
1 error generated.
$ clang++ -fsycl example.cpp
example.cpp:1:1: error: unknown type name 'SYCL_EXTERNAL'
    1 | SYCL_EXTERNAL int mul(int x) {
      | ^
1 error generated.

Device compiler defines SYCL_EXTERNAL macro, whereas host compiler does not.

To define the macro on the host, defines_elementary.hpp header must be included. https://github.com/intel/llvm/blob/sycl/sycl/include/sycl/detail/defines_elementary.hpp#L39

I found this issue while trying to create a simple library of C++ functions available from the SYCL kernels. My library code does not use any SYCL data structures or any symbols. It uses only SYCL_EXTERNAL macro.
I think the implementation should define this macro consistently either using the compiler or using the library, but not both.

@gmlueck, @tahonermann, @Fznamznon, what do you think?

My personal preference is to use the compiler to define SYCL_EXTERNAL.

To reproduce

No response

Environment

No response

Additional context

No response

[tahonermann]

From a SYCL specification perspective, I think inclusion of sycl/sycl.hpp is required for the macros specified in SYCL 2020 section 5.6 ("Preprocessor directives and macros") to be defined. The specification isn't explicit about this, but it is implied by section 3.12.3 ("Library-only implementation"); a library-only implementation has no ability to predefine macros.

From a DPC++ perspective, I tend to agree that, if SYCL_EXTERNAL is predefined for device compilation, then it should probably be defined for host compilation too. However, support for third party host compilers should be considered.

It isn't convenient for me to check right now; how are other SYCL macros like SYCL_FEATURE_SET_FULL and SYCL_FEATURE_SET_REDUCED handled? Are they similarly predefined for device compilation and not predefined for host compilation? Does the behavior differ when using a third party host compiler?

[gmlueck]

From a spec point of view, I think this statement from section 4.3 "Header files and namespace" is clearest:

SYCL provides one standard header file: <sycl/sycl.hpp>, which needs to be included in every translation unit that uses the SYCL programming API.

The macro SYCL_EXTERNAL is part of the "SYCL programming API", so any translation unit using that macro must include the header.

From the spec point of view, this does not preclude the implementation from predefining the macro in the compiler. Rather, it is a requirement imposed on application writers.

Putting on my implementor's hat ... it does seem reasonable that we should define the macro in the same way for both host and device compilation passes. Is there a problem with defining it always in the header for host and device passes? As you point out, this would work also when there is a third party host compiler.

[aelovikov-intel]

I would also argue that -fsycl-device-only is an implementation detail and only the full clang++ -fsycl/clang++ -fsycl -fsycl-host-compiler=<...> are the user-facing interfaces.

I also think that, in general, we should have freedom to emit some errors only in device/host compilation and not in the other (e.g., to be able to save compile time by avoiding doing the same costly processing twice). That doesn't apply directly to this case, but indirectly it shows that device/host can behave differently and there is nothing inherently wrong with that.

Based on that, I think bug is a wrong category. Task/sighting/refactoring would be a better fit.

[tahonermann]

For reference, here is where SYCL_EXTERNAL is predefined for device compilation:

llvm/clang/lib/Frontend/InitPreprocessor.cpp

Lines 1535 to 1538 in f08e857

	if (LangOpts.SYCLIsDevice) {
	Builder.defineMacro("__SYCL_DEVICE_ONLY__");
	if (LangOpts.GPURelocatableDeviceCode)
	Builder.defineMacro("SYCL_EXTERNAL", "__attribute__((sycl_device))");

And here is where it is defined in the SYCL headers:

llvm/sycl/include/sycl/detail/defines_elementary.hpp

Lines 27 to 41 in f08e857

	#ifdef SYCL_EXTERNAL
	#ifdef __NativeCPU__
	#define __DPCPP_SYCL_EXTERNAL SYCL_EXTERNAL __attribute__((__libclc_call__))
	#define __DPCPP_SYCL_EXTERNAL_LIBC SYCL_EXTERNAL
	#else
	#define __DPCPP_SYCL_EXTERNAL SYCL_EXTERNAL
	#endif
	#else
	#ifdef __SYCL_DEVICE_ONLY__
	#define __DPCPP_SYCL_EXTERNAL __attribute__((sycl_device))
	#else
	#define __DPCPP_SYCL_EXTERNAL
	#define SYCL_EXTERNAL
	#endif
	#endif

I'm curious what the motivation is/was for conditionally defining this macro from several possible sources like this. It seems rather convoluted. Note that __CLANG_RDC__ is defined if LangOpts.GPURelocatableDeviceCode is true, so the condition for the predefined case can be checked in the SYCL header file.