[SYCL] Move devicelib assert wrapper to header files

[asavonic]

Object file is no longer required to enable devicelib assert function.
Support for other devicelib functions will follow.

Signed-off-by: Andrew Savonichev [email protected]

[asavonic]

This approach has a limitation: since these wrappers are defined in the header file, it has to be #included in the translation unit that uses standard C and C++ functions. CL/sycl.hpp includes the necessary header, so most of the use-cases should work fine.

If CL/sycl.hpp is not included and standard functions are used in device code (e.g. in functions defined as SYCL_EXTERNAL), user have to manually add #include directive or -include compiler option.

[bader]

Can we put the necessary parts to the compiler headers?
Something similar to https://github.com/intel/llvm/blob/sycl/clang/lib/Headers/__clang_cuda_libdevice_declares.h?

[asavonic]

We can, but why do we need to do this?

[bader]

To lift the limitation you described in this comment.

[vzakhari]

The benefit of moving this to "compiler headers" is that the wrappers will be automatically engaged, when a standard header (e.g. assert.h) is included.

So the new assert.h would provide the implementation for __assert_fail, and then #include_next the real assert.h. We would need a new sub-directory in the compiler headers directory, e.g. device_wrappers, where we will put the new header files.

This will be transparent for programming models that do not require sycl.hpp, but also rely on the devicelib implementation.

At the same time, I am concerned about this change. Let's say we do the same for FP64 math. All wrappers will declare some functions with 'double' arguments, and the functions will remain in the final image at O0. As I understand, we want to make FP64 usage conditional. How do we do that with the header files implementation approach?

[jinge90]

Hi, Slava.
libstdc++, libc++ and MSVC complex implementation are different:
libstdc++: based on c99 complex API, so we need to provide all c99 complex math functions in libdevice lib

libc++: all implementation are included in header file but depends on libm function, so we need to support cmath functions in devicelib to enable.

MSVC: all implementation are included in header file but depends on libm function and some MSVC internal math functions.

For functions included in SPV, we expect backend may provide some optimized version to override and we don't think backend would like to override those MSVC internal functions as they are only used by Windows.

[vzakhari]

Thank you for the explanation, Ge! I will need to comprehend it tomorrow )

[asavonic]

I tried inline and it turned out to work for cmath functions as those functions are very simple wrapper. But I doubt whether it can work on some complicated implementation and we have a case currently

AFAIR, inline does not force inlining, and a linked device program will have a single definition of an inline function. So this should be identical to having this function in an object file.

[jinge90]

I tried inline and it turned out to work for cmath functions as those functions are very simple wrapper. But I doubt whether it can work on some complicated implementation and we have a case currently

AFAIR, inline does not force inlining, and a linked device program will have a single definition of an inline function. So this should be identical to having this function in an object file.

Hi, Andrew.
I think it doesn't matter whether compiler decide to inline the functions or not. We split the device libraries into float and double to support devices without fp64 support. If we move all the implementation into header files, users will include both float and double math functions in their source code even they don't use double in device code and this may introduce unnecessary "double" code in their spirv files which will lead to failures in devices without fp64 support.
If we added "inline" to decorate the functions, I find the functions not invoked in device code will not exist in user's spirv file which seems to be able to solve the problem. But as we know, compiler may deny the "inline" if the functions are too complicated, I am not sure whether adding "inline" decoration can solve the problems under such circumstances, I will try it later.
Thank you very much.

[asavonic]

If we move all the implementation into header files, users will include both float and double math functions in their source code even they don't use double in device code and this may introduce unnecessary "double" code in their spirv files which will lead to failures in devices without fp64 support.
If we added "inline" to decorate the functions, I find the functions not invoked in device code will not exist in user's spirv file which seems to be able to solve the problem.

So basically we have the following:

// used in a kernel
inline float cosf(float f) {
 ... 
}

// not used
inline double cos(double d) {
  ...
}

void kernel() {
  cosf(...);
}

From my understanding, if a function is not called from a kernel, it is not emitted to device LLVM IR (regardless of whether it is inline or not).

[vzakhari]

One drawback of moving the "fallback" implementation into the header file is that it may require -fdeclare-spirv-builtins (#1384) for compilation of the user program. This does not affect this particular change, but may be a problem for following the same idea with the rest of the debicelib fallback implementations.

[asavonic]

One drawback of moving the "fallback" implementation into the header file is that it may require -fdeclare-spirv-builtins (#1384) for compilation of the user program. This does not affect this particular change, but may be a problem for following the same idea with the rest of the debicelib fallback implementations.

-fdeclare-spirv-builtins seems to be enabled by default. Why it is a drawback?

[bader]

-fdeclare-spirv-builtins seems to be enabled by default.

I think it's enabled by default only in the SYCL mode.

[jinge90]

Hi, @asavonic
According to latest code, you want to override system header such as assert.h? To assert, I think that should work since is very simple, it just "include <assert.h>". So in fact it is ours "assert.h" being included in device compilation, am I correct?
However, I am not sure whether it will work for and . Taking libstdc++ as example:
For , it include hierarchy is much more complicated than assert. The will not "include <math.h>" but "include_next<math.h>" as there is a math.h header located in the same folder as cmath header. If we put our math.h in CL/sycl/devicelib and use "-I" to set the include path, it will be skipped.
For , it includes following headers:
bits/c++config.h, bits/cpp_type_traits.h, ext/type_traits, cmath, sstream. As we know, libstdc++ complex math is based on c99 complex but it doesn't include header files like: complex.h or c99complex.h...
This is because all those c99 complex API are invoked via compiler built-in such as __builtin_csin.
Thank you very much.

[asavonic]

Hi, @asavonic
According to latest code, you want to override system header such as assert.h? To assert, I think that should work since is very simple, it just "include <assert.h>". So in fact it is ours "assert.h" being included in device compilation, am I correct?

Right. Our assert.h then includes system assert.h and provides definitions for the library functions.

However, I am not sure whether it will work for and . Taking libstdc++ as example:
For , it include hierarchy is much more complicated than assert. The will not "include <math.h>" but "include_next<math.h>" as there is a math.h header located in the same folder as cmath header. If we put our math.h in CL/sycl/devicelib and use "-I" to set the include path, it will be skipped.

What if we provide our cmath that has #include_next <cmath?

For , it includes following headers:
bits/c++config.h, bits/cpp_type_traits.h, ext/type_traits, cmath, sstream. As we know, libstdc++ complex math is based on c99 complex but it doesn't include header files like: complex.h or c99complex.h...
This is because all those c99 complex API are invoked via compiler built-in such as __builtin_csin.
Thank you very much.

Basically, the idea is to provide function definitions whenever user includes a header that contains the corresponding declarations. If math declarations come from complex header, we should provide complex wrapper. Does it make sense?

[jinge90]

Hi, @asavonic
According to latest code, you want to override system header such as assert.h? To assert, I think that should work since is very simple, it just "include <assert.h>". So in fact it is ours "assert.h" being included in device compilation, am I correct?

Right. Our assert.h then includes system assert.h and provides definitions for the library functions.

However, I am not sure whether it will work for and . Taking libstdc++ as example:
For , it include hierarchy is much more complicated than assert. The will not "include <math.h>" but "include_next<math.h>" as there is a math.h header located in the same folder as cmath header. If we put our math.h in CL/sycl/devicelib and use "-I" to set the include path, it will be skipped.

What if we provide our cmath that has #include_next <cmath?

For , it includes following headers:
bits/c++config.h, bits/cpp_type_traits.h, ext/type_traits, cmath, sstream. As we know, libstdc++ complex math is based on c99 complex but it doesn't include header files like: complex.h or c99complex.h...
This is because all those c99 complex API are invoked via compiler built-in such as __builtin_csin.
Thank you very much.

Basically, the idea is to provide function definitions whenever user includes a header that contains the corresponding declarations. If math declarations come from complex header, we should provide complex wrapper. Does it make sense?

What if we provide our cmath that has #include_next <cmath?
I think include_next should work. At least, to any C++ standard header that can't work in device, we can provide a full implementation to override system's headers, taking cmath as example:
ifdef SYCL_DEVICE_ONLY
//Our which can work in device
else
include_next //We use system cmath in host side
end
In fact, we need to implement everything in if using this solution and the implementation must be compatible with system headers. For example, if we provide a , we must make sure our complex is compatible with libstdc++'s std::complex on Linux and is compatible with MSVC's std::complex on Windows and if users wants to switch to libc++, we also need to keep the compatibility with LLVM libc++.
And I am thinking that it looks like we don't need device library infrastructure any more if we use the system header solution if we don't consider the potential backend optimization in runtime.
By the way, Andy is driving the freestanding library solution recently and if we want to provide headers to override system C++ std headers, maybe we can consider the freestanding library.
@andykaylor , do you have any comments?
Thank you very much.

[asavonic]

What if we provide our cmath that has #include_next <cmath?
I think include_next should work. At least, to any C++ standard header that can't work in device, we can provide a full implementation to override system's headers, taking cmath as example:
ifdef SYCL_DEVICE_ONLY
//Our which can work in device
else
include_next //We use system cmath in host side
end
In fact, we need to implement everything in if using this solution and the implementation must be compatible with system headers.

That is a bit different from what I tried to suggest:

devicelib/cmath:

// Include system header first
#include_next <cmath>

// Provide definitions for library functions
#ifdef __SYCL_DEVICE_ONLY
DEVICE_EXTERN_C inline
float scalbnf(float x, int n) { return __devicelib_scalbnf(x, n); }
#endif

[jinge90]

What if we provide our cmath that has #include_next <cmath?
I think include_next should work. At least, to any C++ standard header that can't work in device, we can provide a full implementation to override system's headers, taking cmath as example:
ifdef SYCL_DEVICE_ONLY
//Our which can work in device
else
include_next //We use system cmath in host side
end
In fact, we need to implement everything in if using this solution and the implementation must be compatible with system headers.

That is a bit different from what I tried to suggest:

devicelib/cmath:

// Include system header first
#include_next <cmath>

// Provide definitions for library functions
#ifdef __SYCL_DEVICE_ONLY
DEVICE_EXTERN_C inline
float scalbnf(float x, int n) { return __devicelib_scalbnf(x, n); }
#endif

Oh, I see. I misunderstood your suggestion. Based the code above, I think it should work. All undefined math function reference should be resolved as long as we let compiler sees the corresponding implementation in device compilation phase. I will try the cmath and complex tomorrow.
Thank you very much.

[jinge90]

What if we provide our cmath that has #include_next <cmath?
I think include_next should work. At least, to any C++ standard header that can't work in device, we can provide a full implementation to override system's headers, taking cmath as example:
ifdef SYCL_DEVICE_ONLY
//Our which can work in device
else
include_next //We use system cmath in host side
end
In fact, we need to implement everything in if using this solution and the implementation must be compatible with system headers.

That is a bit different from what I tried to suggest:

devicelib/cmath:

// Include system header first
#include_next <cmath>

// Provide definitions for library functions
#ifdef __SYCL_DEVICE_ONLY
DEVICE_EXTERN_C inline
float scalbnf(float x, int n) { return __devicelib_scalbnf(x, n); }
#endif

What if we provide our cmath that has #include_next <cmath?
I think include_next should work. At least, to any C++ standard header that can't work in device, we can provide a full implementation to override system's headers, taking cmath as example:
ifdef SYCL_DEVICE_ONLY
//Our which can work in device
else
include_next //We use system cmath in host side
end
In fact, we need to implement everything in if using this solution and the implementation must be compatible with system headers.

That is a bit different from what I tried to suggest:

devicelib/cmath:

// Include system header first
#include_next <cmath>

// Provide definitions for library functions
#ifdef __SYCL_DEVICE_ONLY
DEVICE_EXTERN_C inline
float scalbnf(float x, int n) { return __devicelib_scalbnf(x, n); }
#endif

Hi, @asavonic .
I tried to override cmath and complex on Linux platform, it could work. I found using following command to build the assert.cpp will lead to compiling error:
clang++ -fsycl-device-only -std=c+=11 -S -emit-llvm assert.cpp -o assert.ll -I $devicelib_path
The error message is "undefined reference __devicelib_assert can't be used in SYCL kernel".
The problem is "CL_SYCL_LANGUAGE_VERSION" is not defined when using -fsycl-device-only to build device code only, so the DEVICE_EXTERNAL will be an empty macro.
If we move all wrapper implementation to headers instead of LLVM IR, I think we don't need to add "attribute((weak))" any more as all our implementation will exist in headers and users can't override them if they use -I$devicelib_path to include our headers. We have a test "math_override_test" in llvm/sycl/test/devicelib/ to test user's overriding and it can't work if we use headers. The test uses 2 math functions: sinf and cosf in kernel, user provides his own sinf in source file to override device libraries' sinf and uses device library's cosf. Such mixture will not be supported if we use headers. If users wants to provide implementation for some math functions, he can't include our or multiple definition error will be reported. I think such mixture is corner case and it is OK that we don't support it. So if users wants to use math functions, they have 2 options:

1. use device libraries and adding -I $devicelib_path to include our headers.
2. provide their own implementation, they can't use -I $devicelib_path.

Thank you very much.

[vzakhari]

What if we provide our cmath that has #include_next <cmath?
I think include_next should work. At least, to any C++ standard header that can't work in device, we can provide a full implementation to override system's headers, taking cmath as example:
ifdef SYCL_DEVICE_ONLY
//Our which can work in device
else
include_next //We use system cmath in host side
end
In fact, we need to implement everything in if using this solution and the implementation must be compatible with system headers.

That is a bit different from what I tried to suggest:

devicelib/cmath:

// Include system header first
#include_next <cmath>

// Provide definitions for library functions
#ifdef __SYCL_DEVICE_ONLY
DEVICE_EXTERN_C inline
float scalbnf(float x, int n) { return __devicelib_scalbnf(x, n); }
#endif

What if we provide our cmath that has #include_next <cmath?
I think include_next should work. At least, to any C++ standard header that can't work in device, we can provide a full implementation to override system's headers, taking cmath as example:
ifdef SYCL_DEVICE_ONLY
//Our which can work in device
else
include_next //We use system cmath in host side
end
In fact, we need to implement everything in if using this solution and the implementation must be compatible with system headers.

That is a bit different from what I tried to suggest:

devicelib/cmath:

// Include system header first
#include_next <cmath>

// Provide definitions for library functions
#ifdef __SYCL_DEVICE_ONLY
DEVICE_EXTERN_C inline
float scalbnf(float x, int n) { return __devicelib_scalbnf(x, n); }
#endif

Hi, @asavonic .
I tried to override cmath and complex on Linux platform, it could work. I found using following command to build the assert.cpp will lead to compiling error:
clang++ -fsycl-device-only -std=c+=11 -S -emit-llvm assert.cpp -o assert.ll -I $devicelib_path
The error message is "undefined reference __devicelib_assert can't be used in SYCL kernel".
The problem is "CL_SYCL_LANGUAGE_VERSION" is not defined when using -fsycl-device-only to build device code only, so the DEVICE_EXTERNAL will be an empty macro.
If we move all wrapper implementation to headers instead of LLVM IR, I think we don't need to add "attribute((weak))" any more as all our implementation will exist in headers and users can't override them if they use -I$devicelib_path to include our headers. We have a test "math_override_test" in llvm/sycl/test/devicelib/ to test user's overriding and it can't work if we use headers. The test uses 2 math functions: sinf and cosf in kernel, user provides his own sinf in source file to override device libraries' sinf and uses device library's cosf. Such mixture will not be supported if we use headers. If users wants to provide implementation for some math functions, he can't include our or multiple definition error will be reported. I think such mixture is corner case and it is OK that we don't support it. So if users wants to use math functions, they have 2 options:

1. use device libraries and adding -I $devicelib_path to include our headers.
2. provide their own implementation, they can't use -I $devicelib_path.

Thank you very much.

In general, we cannot request users to add header file paths explicitly. This breaks the current best known practices, which is to let compiler driver manage this.

I still do not like the whole idea of moving implementations to the header files:1

1. Implementations relying on __spirv builtins require -fdeclare-spirv-builtins, which is not supposed to be enabled by default for all languages/programming models.
2. We will have to use __attribute__((weak)) for all definitions, otherwise they will conflict with themselves being included in multiple modules.
3. With the header files implementations we break the current "working" scheme, where user provides own defition(s). For example:

#include <stdio.h>
#include <stdlib.h>

extern inline __attribute__ ((__nothrow__ )) __attribute__ ((__noreturn__)) void __assert_fail (
    const char *__assertion, const char *__file,
    unsigned int __line, const char *__function) {
  printf("my assert\n");
  exit(1);
}

int main() {
  assert(0);
  return 0;
}

This code just works on x86 Linux right now. We are breaking this with the headers implementation, since the user will not be able to redefine __assert_fail. It will still work BC "libraries", since the __assert_fail will be a weak definition there.

If we are concerned about multiple different implementations of assert() in different "libc" libraries, we'd better come up with a way for linking different BC libraries in the driver. So far we only have Linux and MS BC libraries for assert(), so it is not a big problem to link them in clang driver by default (or with some device-only options).

[bader]

Another option is move this header to the compiler headers.
Here the same wrapper implementation for PTX target: https://github.com/intel/llvm/blob/sycl/clang/lib/Headers/__clang_cuda_runtime_wrapper.h#L335.

[bader]

I think this is not required if we rename this header to something like __spir_assert.h and include it any time compilation for spir target is done. This can be achieved if we move the header to the compiler headers as suggested above.

[asavonic]

So clang driver will have to add -include $CLANG_LIB/__spir_assert.h options for all device headers we support - is that what you propose?

For the record, there are two minor disadvantages:

• we have to patch clang driver to add a new header
• all headers must be included - currently they contain ~500 LOC combined

[bader]

So clang driver will have to add -include $CLANG_LIB/__spir_assert.h options for all device headers we support - is that what you propose?

Something like this: https://github.com/intel/llvm/blob/sycl/clang/lib/Driver/ToolChains/Cuda.cpp#L247

For the record, there are two minor disadvantages:

• we have to patch clang driver to add a new header
• all headers must be included - currently they contain ~500 LOC combined

I see these as disadvantages.

[asavonic]

For the record, there are two minor disadvantages:

• we have to patch clang driver to add a new header
• all headers must be included - currently they contain ~500 LOC combined

I see these as disadvantages.

Sorry, it is not clear whether you're in favor of __spir_assert.h or assert.h + #include_next.

[bader]

Sorry, I've made a typo in this comment. I don't think these the items are disadvantages of the approach.

[bader]

In general, we cannot request users to add header file paths explicitly. This breaks the current best known practices, which is to let compiler driver manage this.

Agree. I left a comment above how to addresses this issue.

I still do not like the whole idea of moving implementations to the header files:1

1. Implementations relying on __spirv builtins require -fdeclare-spirv-builtins, which is not supposed to be enabled by default for all languages/programming models.

I think -fdeclare-spirv-builtins must be enabled when we compile for SPIR target, as it provides access to additional functionality provided by SPIR target, which can't be accessed w/o built-ins.

2. We will have to use attribute((weak)) for all definitions, otherwise they will conflict with themselves being included in multiple modules.

I don't follow this comment. Could you clarify what definitions do you mean? This patch includes only wrapper definition and there is no need to use __attribute__((weak)) for this definition. Standard C++ functionality line static and inline should be enough. Right?

3. With the header files implementations we break the current "working" scheme, where user provides own defition(s). For example:

#include <stdio.h>
#include <stdlib.h>

extern inline __attribute__ ((__nothrow__ )) __attribute__ ((__noreturn__)) void __assert_fail (
    const char *__assertion, const char *__file,
    unsigned int __line, const char *__function) {
  printf("my assert\n");
  exit(1);
}

int main() {
  assert(0);
  return 0;
}

This code just works on x86 Linux right now. We are breaking this with the headers implementation, since the user will not be able to redefine __assert_fail. It will still work BC "libraries", since the __assert_fail will be a weak definition there.

This is not supposed to work. __assert_fail is reserved name for the compiler needs. User is allowed to re-define assert name, but defining __assert_fail is UB.

If we are concerned about multiple different implementations of assert() in different "libc" libraries, we'd better come up with a way for linking different BC libraries in the driver. So far we only have Linux and MS BC libraries for assert(), so it is not a big problem to link them in clang driver by default (or with some device-only options).

[bader]

Overall approach looks good to me. 👍
I think we just need to move compiler built-ins (i.e. functions with names starting with underscore) from the library to the compiler.

[asavonic]

extern inline __attribute__ ((__nothrow__ )) __attribute__ ((__noreturn__)) void __assert_fail (
    const char *__assertion, const char *__file,
    unsigned int __line, const char *__function) {
  printf("my assert\n");
  exit(1);
}

int main() {
  assert(0);
  return 0;
}

This code just works on x86 Linux right now. We are breaking this with the headers implementation, since the user will not be able to redefine __assert_fail. It will still work BC "libraries", since the __assert_fail will be a weak definition there.

This is not supposed to work. __assert_fail is reserved name for the compiler needs. User is allowed to re-define assert name, but defining __assert_fail is UB.

What about sin, cos an other math functions? They are currently defined as weak to allow such redefinition. If we follow the approach proposed in this PR, this feature will not work.

[bader]

extern inline __attribute__ ((__nothrow__ )) __attribute__ ((__noreturn__)) void __assert_fail (
    const char *__assertion, const char *__file,
    unsigned int __line, const char *__function) {
  printf("my assert\n");
  exit(1);
}

int main() {
  assert(0);
  return 0;
}

This code just works on x86 Linux right now. We are breaking this with the headers implementation, since the user will not be able to redefine __assert_fail. It will still work BC "libraries", since the __assert_fail will be a weak definition there.

This is not supposed to work. __assert_fail is reserved name for the compiler needs. User is allowed to re-define assert name, but defining __assert_fail is UB.

What about sin, cos an other math functions? They are currently defined as weak to allow such redefinition. If we follow the approach proposed in this PR, this feature will not work.

I'm not sure I understand why these functions should "allow redefintion". Shouldn't single definition be enough?

Math functions definitions for offload targets in llorg use static and always_inline attribute instead: https://github.com/intel/llvm/blob/sycl/clang/lib/Headers/__clang_cuda_cmath.h.

What if we add a compiler header similar to __clang_cuda_cmath.h with following definitions:

#ifdef __SYCL_DEVICE_ONLY__
static __attribute__((always_inline)) float cos(float __x) { return __devicelib_cosf(__x); }
...
#endif

?
Do you see any problems with it?

Regardless whether we decide to refactor math functions or not, I think we should merge this PR.

[vzakhari]

In general, we cannot request users to add header file paths explicitly. This breaks the current best known practices, which is to let compiler driver manage this.

Agree. I left a comment above how to addresses this issue.

I still do not like the whole idea of moving implementations to the header files:1

1. Implementations relying on __spirv builtins require -fdeclare-spirv-builtins, which is not supposed to be enabled by default for all languages/programming models.

I think -fdeclare-spirv-builtins must be enabled when we compile for SPIR target, as it provides access to additional functionality provided by SPIR target, which can't be accessed w/o built-ins.

OK, I think we can imply -fdeclare-spirv-builtins.

1. We will have to use attribute((weak)) for all definitions, otherwise they will conflict with themselves being included in multiple modules.

I don't follow this comment. Could you clarify what definitions do you mean? This patch includes only wrapper definition and there is no need to use __attribute__((weak)) for this definition. Standard C++ functionality line static and inline should be enough. Right?

You are right.

1. With the header files implementations we break the current "working" scheme, where user provides own defition(s). For example:

#include <stdio.h>
#include <stdlib.h>

extern inline __attribute__ ((__nothrow__ )) __attribute__ ((__noreturn__)) void __assert_fail (
    const char *__assertion, const char *__file,
    unsigned int __line, const char *__function) {
  printf("my assert\n");
  exit(1);
}

int main() {
  assert(0);
  return 0;
}

This code just works on x86 Linux right now. We are breaking this with the headers implementation, since the user will not be able to redefine __assert_fail. It will still work BC "libraries", since the __assert_fail will be a weak definition there.

This is not supposed to work. __assert_fail is reserved name for the compiler needs. User is allowed to re-define assert name, but defining __assert_fail is UB.

If we are concerned about multiple different implementations of assert() in different "libc" libraries, we'd better come up with a way for linking different BC libraries in the driver. So far we only have Linux and MS BC libraries for assert(), so it is not a big problem to link them in clang driver by default (or with some device-only options).

I am just saying that it is working right now, even though it is UB. As Andrew mentioned, sin() and cos() is another example where it is currently possible. If we are OK with breaking support for UB programs, then it is not a problem and we can use the headers solution.

[jinge90]

extern inline __attribute__ ((__nothrow__ )) __attribute__ ((__noreturn__)) void __assert_fail (
    const char *__assertion, const char *__file,
    unsigned int __line, const char *__function) {
  printf("my assert\n");
  exit(1);
}

int main() {
  assert(0);
  return 0;
}

This code just works on x86 Linux right now. We are breaking this with the headers implementation, since the user will not be able to redefine __assert_fail. It will still work BC "libraries", since the __assert_fail will be a weak definition there.

This is not supposed to work. __assert_fail is reserved name for the compiler needs. User is allowed to re-define assert name, but defining __assert_fail is UB.

What about sin, cos an other math functions? They are currently defined as weak to allow such redefinition. If we follow the approach proposed in this PR, this feature will not work.

Yes, if we merge this PR, users can't provide their own version functions to override the corresponding one in device library. If we can accept this, it is OK.

[jinge90]

What if we provide our cmath that has #include_next <cmath?
I think include_next should work. At least, to any C++ standard header that can't work in device, we can provide a full implementation to override system's headers, taking cmath as example:
ifdef SYCL_DEVICE_ONLY
//Our which can work in device
else
include_next //We use system cmath in host side
end
In fact, we need to implement everything in if using this solution and the implementation must be compatible with system headers.

That is a bit different from what I tried to suggest:

devicelib/cmath:

// Include system header first
#include_next <cmath>

// Provide definitions for library functions
#ifdef __SYCL_DEVICE_ONLY
DEVICE_EXTERN_C inline
float scalbnf(float x, int n) { return __devicelib_scalbnf(x, n); }
#endif

What if we provide our cmath that has #include_next <cmath?
I think include_next should work. At least, to any C++ standard header that can't work in device, we can provide a full implementation to override system's headers, taking cmath as example:
ifdef SYCL_DEVICE_ONLY
//Our which can work in device
else
include_next //We use system cmath in host side
end
In fact, we need to implement everything in if using this solution and the implementation must be compatible with system headers.

That is a bit different from what I tried to suggest:

devicelib/cmath:

// Include system header first
#include_next <cmath>

// Provide definitions for library functions
#ifdef __SYCL_DEVICE_ONLY
DEVICE_EXTERN_C inline
float scalbnf(float x, int n) { return __devicelib_scalbnf(x, n); }
#endif

Hi, @asavonic .
I tried to override cmath and complex on Linux platform, it could work. I found using following command to build the assert.cpp will lead to compiling error:
clang++ -fsycl-device-only -std=c+=11 -S -emit-llvm assert.cpp -o assert.ll -I $devicelib_path
The error message is "undefined reference __devicelib_assert can't be used in SYCL kernel".
The problem is "CL_SYCL_LANGUAGE_VERSION" is not defined when using -fsycl-device-only to build device code only, so the DEVICE_EXTERNAL will be an empty macro.
If we move all wrapper implementation to headers instead of LLVM IR, I think we don't need to add "attribute((weak))" any more as all our implementation will exist in headers and users can't override them if they use -I$devicelib_path to include our headers. We have a test "math_override_test" in llvm/sycl/test/devicelib/ to test user's overriding and it can't work if we use headers. The test uses 2 math functions: sinf and cosf in kernel, user provides his own sinf in source file to override device libraries' sinf and uses device library's cosf. Such mixture will not be supported if we use headers. If users wants to provide implementation for some math functions, he can't include our or multiple definition error will be reported. I think such mixture is corner case and it is OK that we don't support it. So if users wants to use math functions, they have 2 options:

1. use device libraries and adding -I $devicelib_path to include our headers.
2. provide their own implementation, they can't use -I $devicelib_path.

Thank you very much.

In general, we cannot request users to add header file paths explicitly. This breaks the current best known practices, which is to let compiler driver manage this.

I still do not like the whole idea of moving implementations to the header files:1

1. Implementations relying on __spirv builtins require -fdeclare-spirv-builtins, which is not supposed to be enabled by default for all languages/programming models.
2. We will have to use attribute((weak)) for all definitions, otherwise they will conflict with themselves being included in multiple modules.
3. With the header files implementations we break the current "working" scheme, where user provides own defition(s). For example:

#include <stdio.h>
#include <stdlib.h>

extern inline __attribute__ ((__nothrow__ )) __attribute__ ((__noreturn__)) void __assert_fail (
    const char *__assertion, const char *__file,
    unsigned int __line, const char *__function) {
  printf("my assert\n");
  exit(1);
}

int main() {
  assert(0);
  return 0;
}

This code just works on x86 Linux right now. We are breaking this with the headers implementation, since the user will not be able to redefine __assert_fail. It will still work BC "libraries", since the __assert_fail will be a weak definition there.

If we are concerned about multiple different implementations of assert() in different "libc" libraries, we'd better come up with a way for linking different BC libraries in the driver. So far we only have Linux and MS BC libraries for assert(), so it is not a big problem to link them in clang driver by default (or with some device-only options).

I think adding the device library(wrapper library) in driver is another option. The work can be split into following steps:

1. we build the wrapper library(current .o file) directly to LLVM IR(.bc) files.
2. In llvm-link phase, we link all device library LLVM .bc files with users' .bc files to get final device code.(we need to add those device library LLVM .bc files in driver's llvm-link phase)
3. llvm-link may be not so "clever" and will pull in many functions unused, we can remove all unused functions in the LLVM IR generated by llvm-link.
4. llvm-spirv and other steps.
   After doing this, user don't need to add device libraries into their building command. And users can still provide their own functions to override the corresponding ones in device library.
   Thank you very much.

[andykaylor]

I am concerned about the inability of users to provide their own definitions of functions like sin, cos, etc. For functions that begin with __ that's OK, because the user isn't allowed to use that prefix for their symbols. For math library functions, it must be allowed.

It also worries me that we are violating the one definition rule here. I suppose the device library implementations do that already to some extent, but there at least it's happening at the object level which is outside of C++ standards scope. This introduces multiple definitions at the source level.

I'd like to see input from some C/C++ experts on this.

[erichkeane]

I am concerned about the inability of users to provide their own definitions of functions like sin, cos, etc. For functions that begin with __ that's OK, because the user isn't allowed to use that prefix for their symbols. For math library functions, it must be allowed.

It also worries me that we are violating the one definition rule here. I suppose the device library implementations do that already to some extent, but there at least it's happening at the object level which is outside of C++ standards scope. This introduces multiple definitions at the source level.

I'd like to see input from some C/C++ experts on this.

Can you clarify your concern? The comment tree above is massive and difficult to figure out what is happening. If we're providing a custom cmath (as the implementation, naming a file this is UB unless we are considered 'the implementation') we're responsible for ensuring that the entire standard library is provided. We're also responsible for making sure that every translation unit in a program sees the same definition of each function.

Users cannot define functions defined in the standard library (nothing in namespace std, and nothing using identifiers reserved by C, including these names). When included in the C++ standard, it is unclear whether these names in the global namespace are still reserved however.

[andykaylor]

I am concerned about the inability of users to provide their own definitions of functions like sin, cos, etc. For functions that begin with __ that's OK, because the user isn't allowed to use that prefix for their symbols. For math library functions, it must be allowed.
Can you clarify your concern?

Here my concern is that if the user wants to have a function named sin() that can be called instead of the standard library function, that should be allowed. It doesn't even need to have the same semantics as the standard library function. It could print "Hello world" if it wanted. The comments above led me to believe that the current implementation somehow disallows this.

I don't know if the language standard allows this, but in practice it is done and is the reason the nobuiltin attribute and the -fno-builtin command line option exist.

It also worries me that we are violating the one definition rule here. I suppose the device library implementations do that already to some extent, but there at least it's happening at the object level which is outside of C++ standards scope. This introduces multiple definitions at the source level.
Can you clarify your concern?

Here my concern is a bit more pedantic, but could have practical consequences. If we've got a header file that does something like this:

#ifdef SYCL_DEVICE
inline void __assert_fail() { doSomething(); }
#else
inline void __assert_fail() { doSomethingDifferent(); }
#endif

and we compile the file with and without that symbol defined, we've broken the one definition rule in a very real way. We might get away with it, but I think we've clearly broken the rule. Does the use of inline make this OK?

[erichkeane]

I am concerned about the inability of users to provide their own definitions of functions like sin, cos, etc. For functions that begin with __ that's OK, because the user isn't allowed to use that prefix for their symbols. For math library functions, it must be allowed.
Can you clarify your concern?

Here my concern is that if the user wants to have a function named sin() that can be called instead of the standard library function, that should be allowed. It doesn't even need to have the same semantics as the standard library function. It could print "Hello world" if it wanted. The comments above led me to believe that the current implementation somehow disallows this.

I don't know if the language standard allows this, but in practice it is done and is the reason the nobuiltin attribute and the -fno-builtin command line option exist.

The language prohibits it (at least with the same signature in C++), but you're right, it IS something people do. That said, its so easy to accidentally include cmath/math.h I'm not sure someone could get away with it for long.

It also worries me that we are violating the one definition rule here. I suppose the device library implementations do that already to some extent, but there at least it's happening at the object level which is outside of C++ standards scope. This introduces multiple definitions at the source level.
Can you clarify your concern?

Here my concern is a bit more pedantic, but could have practical consequences. If we've got a header file that does something like this:

#ifdef SYCL_DEVICE
inline void __assert_fail() { doSomething(); }
#else
inline void __assert_fail() { doSomethingDifferent(); }
#endif

and we compile the file with and without that symbol defined, we've broken the one definition rule in a very real way. We might get away with it, but I think we've clearly broken the rule. Does the use of inline make this OK?

YES, that is technically an ODR violation. Inline doesn't save you, there are still multiple definitions (typically, inline makes the ODR WORSE, since it encourages inlining which leads to not as easily being able to replace the function).

In reality though, the reason the ODR exists (in this case) is to protect the compiler in the case where the linker could choose one of a couple of definitions at random. For example, if you had in your program two versions of the same function in two different TUs, but ALSO had inlining and LTO, you could get a surprising behavior where the definition chosen in each call isn't necessarily the one from the same TU.

The __assert_fail case above is fairly isolated from this problem since we're sort of changing the definition of 'program' as far as the ODR is concerned (at least this is how I sleep at night:) ). A "SYCL Program" is actually N "programs" as far as the language is concerned, each device sees its own 'program', so the ODR gets isolated in the Device vs host situation.

[s-kanaev]

Can this be included somewhere in SYCL headers?

[asavonic]

The point is to support compilation when SYCL headers are not included, e.g. when SYCL_EXTERNAL is used.

[bader]

+1.
The idea here is the compiler to provide additional functionality for SPIR target through the functions declared in this header.
Current implementation is not quite implements this idea - it includes the header for SYCL mode rather then SPIR target, so we probably have to skip this include for non-SPIR targets (e.g. nvidia).

[s-kanaev]

The point is to support compilation when SYCL headers are not included

Compilation of what?

[asavonic]

The point is to support compilation when SYCL headers are not included

Compilation of what?

Compilation of source code that do not #include SYCL headers.

[asavonic]

Current implementation is not quite implements this idea - it includes the header for SYCL mode rather then SPIR target, so we probably have to skip this include for non-SPIR targets (e.g. nvidia).

The last patch includes the header when SYCL language is compiled for SPIR target. Non-SPIR targets should not be affected.

[bader]

Andrew, can we make this include agnostic to compilation mode/language and include it for all compilations for SPIR target?

[asavonic]

If it makes sense - sure. I'm concerned that this feature will be enabled for languages where it doesn't make sense (for OpenCL, in particular).

[bader]

Why do you think it doesn't make sense for OpenCL?

[asavonic]

I think this is ultimately for OpenCL folks to decide. Right now this extension is not documented to work for OpenCL.

[s-kanaev]

This path (i.e. devicelib/) doesn't conflict with anything else, does it?

[asavonic]

Not sure what do you mean by a "conflict". When installed on a system, this directory resides in $prefix/lib/clang/$version/include directory.

[s-kanaev]

OK, then.

[AGindinson]

Just to make sure, would:

Suggested change

	if (getToolChain().getTriple().isSPIR()) {
	if (JA.isDeviceOffloading()) {

be unacceptable because DPC++ NVPTX compilations would also pass the condition?

[AGindinson]

A minor suggestion would be to make this logic part of AddSYCLIncludeArgs() (and get rid of the separate AddDevicelibIncludeArgs())

[AGindinson]

Could this have separate RUN lines, with and without -fsycl-device-only?

[AGindinson]

Suggested change

	// CHECK-SYCL-DEV: "-include" "devicelib{{[/\\]}}__devicelib_assert.h"
	// CHECK-SYCL-DEV: "-fsycl-is-device"{{.*}} "-include" "devicelib{{[/\\]}}__devicelib_assert.h"

[asavonic]

There are two approaches: with #include_next (ff9a212), and with an implicit
-include from the clang driver (464d3a5).

The first approach works fine, and I don't see any disadvantages of it.

The second approach is a bit more tricky:

1. It requires a standard library to be available and configured properly to
   compile SYCL code that doesn't even use the standard library. This doesn't
   hold true at least for some Clang driver LIT tests:

   /usr/include/features.h(424,12): fatal error: 'sys/cdefs.h' file not found
   #  include <sys/cdefs.h>
           ^~~~~~~~~~~~~
   

2. It also requires to split devicelib implementation into two parts: headers in
   llvm/clang/lib/Headers and implementation in llvm/libdevice.

3. spirv_vars.hpp is also moved to llvm/clang/lib/Headers and adapted to C
   compilation (we have LIT tests with this)

[jinge90]

There are two approaches: with #include_next (ff9a212), and with an implicit
-include from the clang driver (464d3a5).

The first approach works fine, and I don't see any disadvantages of it.

The second approach is a bit more tricky:

1. It requires a standard library to be available and configured properly to
   compile SYCL code that doesn't even use the standard library. This doesn't
   hold true at least for some Clang driver LIT tests:

   /usr/include/features.h(424,12): fatal error: 'sys/cdefs.h' file not found
   #  include <sys/cdefs.h>
           ^~~~~~~~~~~~~
   

2. It also requires to split devicelib implementation into two parts: headers in
   llvm/clang/lib/Headers and implementation in llvm/libdevice.
3. spirv_vars.hpp is also moved to llvm/clang/lib/Headers and adapted to C
   compilation (we have LIT tests with this)

Hi, @asavonic
To "assert", the "#include_next" works fine but I am afraid it may be not friendly to other header files with more complicated include hierarchy. As we discussed before, we have to #include_next instead of #include_next<math.h> on Linux platform as libstdc++'s cmath has already used include_next<math.h>. Things may be different on Windows as MSVC cmath header file may not use include_next<math.h>. For , the situation is much more simpler as both libstdc++'s and MSVC's are almost the same only including assert.h.
By the way, if we use include_next, we will introduce dependency on "include hierarchy" of system's C++ header files, if the library developer changes the "include..." in C++ headers, our "include_next" may break. For example, if library developer replaces "#include <assert.h>" with "#include_next<assert.h>" in , our assert.h will be skipped. To , such change has very little probability to happen, but I think it may happen to other headers.
Thank you very much.

[asavonic]

There are two approaches: with #include_next (ff9a212), and with an implicit
-include from the clang driver (464d3a5).
The first approach works fine, and I don't see any disadvantages of it.
The second approach is a bit more tricky:

1. It requires a standard library to be available and configured properly to
   compile SYCL code that doesn't even use the standard library. This doesn't
   hold true at least for some Clang driver LIT tests:

   /usr/include/features.h(424,12): fatal error: 'sys/cdefs.h' file not found
   #  include <sys/cdefs.h>
           ^~~~~~~~~~~~~
   

2. It also requires to split devicelib implementation into two parts: headers in
   llvm/clang/lib/Headers and implementation in llvm/libdevice.
3. spirv_vars.hpp is also moved to llvm/clang/lib/Headers and adapted to C
   compilation (we have LIT tests with this)

Hi, @asavonic
To "assert", the "#include_next" works fine but I am afraid it may be not friendly to other header files with more complicated include hierarchy. As we discussed before, we have to #include_next instead of #include_next<math.h> on Linux platform as libstdc++'s cmath has already used include_next<math.h>. Things may be different on Windows as MSVC cmath header file may not use include_next<math.h>. For , the situation is much more simpler as both libstdc++'s and MSVC's are almost the same only including assert.h.
By the way, if we use include_next, we will introduce dependency on "include hierarchy" of system's C++ header files, if the library developer changes the "include..." in C++ headers, our "include_next" may break. For example, if library developer replaces "#include <assert.h>" with "#include_next<assert.h>" in , our assert.h will be skipped. To , such change has very little probability to happen, but I think it may happen to other headers.
Thank you very much.

Can we provide both cmath and math.h wrappers and #include_next the corresponding header?

[jinge90]

There are two approaches: with #include_next (ff9a212), and with an implicit
-include from the clang driver (464d3a5).
The first approach works fine, and I don't see any disadvantages of it.
The second approach is a bit more tricky:

1. It requires a standard library to be available and configured properly to
   compile SYCL code that doesn't even use the standard library. This doesn't
   hold true at least for some Clang driver LIT tests:

   /usr/include/features.h(424,12): fatal error: 'sys/cdefs.h' file not found
   #  include <sys/cdefs.h>
           ^~~~~~~~~~~~~
   

2. It also requires to split devicelib implementation into two parts: headers in
   llvm/clang/lib/Headers and implementation in llvm/libdevice.
3. spirv_vars.hpp is also moved to llvm/clang/lib/Headers and adapted to C
   compilation (we have LIT tests with this)

Hi, @asavonic
To "assert", the "#include_next" works fine but I am afraid it may be not friendly to other header files with more complicated include hierarchy. As we discussed before, we have to #include_next instead of #include_next<math.h> on Linux platform as libstdc++'s cmath has already used include_next<math.h>. Things may be different on Windows as MSVC cmath header file may not use include_next<math.h>. For , the situation is much more simpler as both libstdc++'s and MSVC's are almost the same only including assert.h.
By the way, if we use include_next, we will introduce dependency on "include hierarchy" of system's C++ header files, if the library developer changes the "include..." in C++ headers, our "include_next" may break. For example, if library developer replaces "#include <assert.h>" with "#include_next<assert.h>" in , our assert.h will be skipped. To , such change has very little probability to happen, but I think it may happen to other headers.
Thank you very much.

Can we provide both cmath and math.h wrappers and #include_next the corresponding header?

Just checked MSVC's cmath header, it doesn't include math.h, so include_next may be the only choice for C++ std math headers. And it looks like is similar.

[jinge90]

Hi, @asavonic .
If compiler developer provides some functions in device library via headers, the compiler users should not try to provide their own version in source code. This "user override" behavior is permitted now as all functions in current device library is "weak". Suggest to tell users that they should not try to override any function provided in header-based device library.

[vzakhari]

There are two approaches: with #include_next (ff9a212), and with an implicit
-include from the clang driver (464d3a5).

The first approach works fine, and I don't see any disadvantages of it.

The second approach is a bit more tricky:

1. It requires a standard library to be available and configured properly to
   compile SYCL code that doesn't even use the standard library. This doesn't
   hold true at least for some Clang driver LIT tests:

   /usr/include/features.h(424,12): fatal error: 'sys/cdefs.h' file not found
   #  include <sys/cdefs.h>
           ^~~~~~~~~~~~~
   

2. It also requires to split devicelib implementation into two parts: headers in
   llvm/clang/lib/Headers and implementation in llvm/libdevice.
3. spirv_vars.hpp is also moved to llvm/clang/lib/Headers and adapted to C
   compilation (we have LIT tests with this)

Just a minor remark, the #include_next approach still requires driver modification, e.g. for OpenMP the include path to devicelib sub-directory in clang headers structure has to be added in the driver. I am OK if you want to proceed with ff9a212, but it has to be fixed so that assert.h is installed into clang headers structure - will this work for you?

[bader]

Agree with Slava, we need either to put the "assert.h" header into clang headers to make driver to add include path(s) to similar devicelib directory with standard headers wrappers before the standard library includes. Otherwise standard "assert.h" will be included.

[s-kanaev]

@asavonic , resolve conflicts