Skip SYCL_EXTERNAL definition when -fno-sycl-rdc is set

[bader]

@sarnex, I'm trying to build SYCL-CTS with -fno-sycl-rdc option and hit a problem.
SYCL-CTS uses SYCL_EXTERNAL macro in the tests. This is an optional feature and to check whether implementation supports it CTS uses following check:

#ifdef SYCL_EXTERNAL
// code using SYCL_EXTERNAL
#endif

This is aligned with the SYCL specification:

By default, any function that is odr-used from a device function must be defined in the same translation unit as that use. However, this restriction is relaxed if both of the following conditions are met:

• The implementation defines the SYCL_EXTERNAL macro;

f884993 keeps definition of SYCL_EXTERNAL macro, but emits a new error message for usages.

What do you think about removing the definition of SYCL_EXTERNAL macro in -fno-sycl-rdc mode? NOTE: this will impact the diagnostics. There still be an error message, but probably less intuitive than exiting diagnostics.

Alternative option would be to change the way users detect whether implementation supports external device function definitions. C++ Feature Test might be an option, but it's C++20 feature and I'm not sure if we can adopt it for SYCL-2020. @gmlueck, FYI.

[AlexeySachkov]

We had a discussion about that here: #7595 (comment)

We agreed that a dedicated diagnostic that SYCL_EXTERNAL can't be used in -fno-sycl-rdc mode is better than generic error message about SYCL_EXTERNAL not being defined. Of course, such specific error message is not required by the spec and that users fault if they code doesn't compliant with the spec, but having such specific error message improves user experience.

However, I agree that we have this mode, which effectively doesn't support SYCL_EXTERNAL, but doesn't provide a mechanism for users to write portable code, i.e. one which can be compiled in both modes. Essentially, -fno-sycl-rdc mode is not compliant with the SYCL 2020 specification.

[sarnex]

Yeah my understanding was we knew the option is not SYCL compliant, but we can get some some big compile-time gains in some workloads so we added support.

I would be happy to implement a change where we don't define SYCL_EXTERNAL in -fno-sycl-rdc mode, but we probably want to get approval from @gmlueck first.

[bader]

Yeah my understanding was we knew the option is not SYCL compliant, but we can get some some big compile-time gains in some workloads so we added support.

To be precise, the implementation is not SYCL compliant, not the option per se. We can have SYCL compliant implementation for the option as well.

With regard to compile-time gains, I see compile-time improvements for SYCL-CTS tests as well. Here are few examples:

(deafult) Linking CXX executable bin/test_all

real    8m17.246s
user    7m24.000s
sys     1m27.855s

(no-rdc) Linking CXX executable bin/test_all

real    5m29.925s
user    4m17.767s
sys     1m27.755s

(default) Linking CXX executable bin/test_accessor

real    0m9.032s
user    0m6.504s
sys     0m2.801s

(no-rdc) Linking CXX executable bin/test_accessor

real    0m12.406s
user    0m7.371s
sys     0m5.508s

(deafult) Linking CXX executable bin/test_vector_swizzles

real    1m27.937s
user    1m19.776s
sys     0m18.150s

(no-rdc) Linking CXX executable bin/test_vector_swizzles

real    0m53.737s
user    0m46.081s
sys     0m7.712s

Notice that for some reason test_accessor link time with -fno-sycl-rdc option increases.

[sarnex]

Good clarification (and data), thanks. If you end up having time it would be interesting to know what's going on with test_accessor, but it may be expected.

[gmlueck]

I think it's better to give an error if the source file has SYCL_EXTERNAL and is compiled with -fno-sycl-rdc. Consider this case:

• Source file "a.cpp" defines a function "foo" with SYCL_EXTERNAL and is compiled with -fno-sycl-rdc.
• Source file "b.cpp" declares (does not define) function "foo" with SYCL_EXTERNAL and is compiled without -fno-sycl-rdc.

With my proposal, the user gets an error when compiling "a.cpp". If we simply undefine SYCL_EXTERNAL when compiling with -fno-sycl-rdc, the above scenario does not produce an obvious error message, and it does not work as expected.

[sarnex]

Just noting that this is the currently implemented behavior, and we have a specific error message

invalid {declaration,definition} of SYCL_EXTERNAL function in non-relocatable device code mode

[bader]

@gmlueck, if we undefine SYCL_EXTERNAL compiler will still produce an error and will be SYCL 2020 spec compliant.
The only downside of this is the error message won't be specialized for SYCL_EXTERNAL macro usage, but will be generic C++ error like:

unknown type name 'SYCL_EXTERNAL'

[gmlueck]

That's a good point. Somehow I was thinking that -fsycl-no-rdc would define SYCL_EXTERNAL to the empty string, but that's not the case. I think you've convinced me that it's better for -fsycl-no-rdc to leave SYCL_EXTERNAL undefined.

[sarnex]

I will make an internal issue for me to implement this change

[sarnex]

@bader @gmlueck I did some initial investigation for this and it seems a little more complicated than we thought.

1. While we do define SYCL_EXTERNAL in the preprocessor here, we also define it explicitly in the SYCL headers here, so just skipping the define/undefining it in the same spot in the preprocessor wouldn't do what we want.

2. Even if we figured out 1, we have a ton of stuff in the SYCL headers that uses SYCL_EXTERNAL, ex one, two
   so we have to do something a little smarter.

Do you guys have and good solutions?

One idea I had was to change the preprocessor to define SYCL_EXTERNAL at the beginning of every file inside the SYCL system directory if -fno-sycl-rdc is passed, and undefine SYCL_EXTERNAL at the end of the file, but this might be a complicated change in the preprocessor (not sure, I haven't worked in it before, maybe it's trivial).
At least I think this would ensure SYCL code can still use it, but user code can't.

Did I miss an obvious solution?

[bader]

Can we change this code https://github.com/intel/llvm/blob/sycl/sycl/include/sycl/detail/defines_elementary.hpp#L30 to something like this?

#ifdef SYCL_EXTERNAL
#define __DPCPP_SYCL_EXTERNAL SYCL_EXTERNAL
#else
#define __DPCPP_SYCL_EXTERNAL
#endif

Use __DPCPP_SYCL_EXTERNAL in DPC++ headers instead of SYCL_EXTERNAL.
Do you expect any issues with we drop SYCL_EXTERNAL when -fno-sycl-rdc is set?

[sarnex]

The root issue is that the front end will error if SYCL_EXTERNAL is not defined to __attribute__((sycl_device)) as we see here:
error: SYCL kernel cannot call an undefined function without SYCL_EXTERNAL attribute

That's from when I tried an example removing SYCL_EXTERNAL from the SYCL header decl of some fcn and then called it in the kernel, it was __imf_saturatef

I guess we could do

#ifdef SYCL_EXTERNAL
#define __DPCPP_SYCL_EXTERNAL SYCL_EXTERNAL
#else
#define __DPCPP_SYCL_EXTERNAL __attribute__((sycl_device))
#endif

and then use __DPCPP_SYCL_EXTERNAL, but if someone didn't notice this pattern while adding a new function and accidentally used SYCL_EXTERNAL, it would only expose the bug if they test with -fno-sycl-rdc which IMO is very unlikely.

There should be no issues with dropping SYCL_EXTERNAL from a functional POV, I already had to skip throwing the error we throw today for uses inside the SYCL system headers. All of these functions should either be provided by the SYCL device libraries, or are expected to be undefined and the SPIRV translator removes the uses completely and replaces them with some SPIR-V intrinsic.

[bader]

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

__attribute__((sycl_device)) can be used on device only.

if someone didn't notice this pattern while adding a new function and accidentally used SYCL_EXTERNAL, it would only expose the bug if they test with -fno-sycl-rdc which IMO is very unlikely.

To catch that we can extend sycl/test/basic_tests/macros.cpp with the run clang++ -fsycl -fno-sycl-rdc and check that SYCL_EXTERNAL is not defined.

There should be no issues with dropping SYCL_EXTERNAL from a functional POV, I already had to skip throwing the error we throw today for uses inside the SYCL system headers. All of these functions should either be provided by the SYCL device libraries, or are expected to be undefined and the SPIRV translator removes the uses completely and replaces them with some SPIR-V intrinsic.

So technically we rely on device code linking to support "C/C++ standard library" extension, but we don't necessarily need to use standard macro and can use compiler attribute instead. 👍

BTW, do you have any idea if we can keep SYCL_EXTERNAL macro undefined and have existing custom diagnostic at the same time?

[sarnex]

attribute((sycl_device)) can be used on device only.

Good catch, thanks

To catch that we can extend sycl/test/basic_tests/macros.cpp with the run clang++ -fsycl -fno-sycl-rdc and check that SYCL_EXTERNAL is not defined.

Yeah this is a good idea, thanks

So technically we rely on device code linking to support "C/C++ standard library" extension, but we don't necessarily need to use standard macro and can use compiler attribute instead

Yes, exactly

BTW, do you have any idea if we can keep SYCL_EXTERNAL macro undefined and have existing custom diagnostic at the same time?

No idea, let me investigate.

This seems like a good solution so I'll try implementing this, thanks again

[sarnex]

PR: #8537