Skip to content

Conversation

jinge90
Copy link
Contributor

@jinge90 jinge90 commented Aug 30, 2023

No description provided.

@jinge90 jinge90 requested a review from a team as a code owner August 30, 2023 14:16
@jinge90 jinge90 requested a review from cperkinsintel August 30, 2023 14:16
@jinge90 jinge90 marked this pull request as draft August 30, 2023 14:16
@jinge90
Copy link
Contributor Author

jinge90 commented Sep 11, 2023

Hi, @steffenlarsen
This draft PR removes all cmath decls from sycl builtins.hpp, we will give green light for these math functions in SYCL sema checking instead. Could you help review and evaluate whether we can proceed with this PR?
Thanks very much.

@jinge90 jinge90 marked this pull request as ready for review September 12, 2023 00:27
@jinge90 jinge90 requested a review from a team as a code owner September 12, 2023 00:27
Copy link
Contributor

@steffenlarsen steffenlarsen left a comment

Choose a reason for hiding this comment

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

I am fine with this if the frontend people are happy with the Sema solution.

Copy link
Contributor

@elizabethandrews elizabethandrews left a comment

Choose a reason for hiding this comment

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

Can you explain why this is required? This seems strange to me. @AaronBallman @premanandrao can you take a look please?

@aelovikov-intel aelovikov-intel self-requested a review September 14, 2023 21:47
@aelovikov-intel
Copy link
Contributor

Putting myself in reviewers just to get the notifications matching my outlook filters, please ignore it.

@steffenlarsen
Copy link
Contributor

Can you explain why this is required? This seems strange to me.

With recent changes to SYCL builtin definitions, having conflicting math builtins in the global namespace may cause ambiguity errors for users with using namespace sycl in their code. Where cmath is included we cannot avoid this, but we should at the very least avoid declaring/including them in the SYCL headers.

@aelovikov-intel
Copy link
Contributor

Can you explain why this is required? This seems strange to me.

With recent changes to SYCL builtin definitions, having conflicting math builtins in the global namespace may cause ambiguity errors for users with using namespace sycl in their code. Where cmath is included we cannot avoid this, but we should at the very least avoid declaring/including them in the SYCL headers.

I'm working on a different approach to this - sycl...aelovikov-intel:llvm:no-complex-include.

if (!Callee->getIdentifier())
return false;

if (LibdeviceCmathSet.count(Callee->getName().str()) &&
Copy link
Contributor

Choose a reason for hiding this comment

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

One thing that confuses me is: aren't all these functions already builtins?

if (!Callee->isDefined() && !Callee->getBuiltinID() &&

https://github.com/intel/llvm/blob/89e2280d53e50346e69ed8354d47a9223f46f483/clang/include/clang/Basic/Builtins.def#L1350C55-L1350C55

Copy link
Contributor

Choose a reason for hiding this comment

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

Is the issue that the sycl versions of these functions don't get treated as builtins, so we need this new table to identify them?

@jinge90
Copy link
Contributor Author

jinge90 commented Sep 18, 2023

Hi, all.
We supported to use cmath/complex functions in sycl kernel code via libdevice long time. If users want to call functions from <cmath>, <complex>, they only need to use these headers and call them just like what they do in normal CPU programs.
These math functions are implemented in libm, if we directly call them in sycl kernel code, sycl semantic checking will report error saying we can't call undefined function without "sycl external" attribute, these math/complex functions' declarations are in system header which we can't modify, so we add declarations for these functions with "sycl external" attribute decorated in sycl builtins.hpp.
However, as @steffenlarsen mentioned:
"With recent changes to SYCL builtin definitions, having conflicting math builtins in the global namespace may cause ambiguity errors for users with using namespace sycl in their code. Where cmath is included we cannot avoid this, but we should at the very least avoid declaring/including them in the SYCL headers."
So, we are trying to remove these math decls in sycl builtins.hpp, the most straightforward approach seems to give green light to these cmath functions which has been supported by libdevice in sycl kernel.
On Linux platform, 's implementation look like:
inline _GLIBCXX_CONSTEXPR float
sqrt(float __x)
{ return __builtin_sqrtf(__x); }
sema checking will not report error for such case since math function will be treated as "builtin" but MSVC's cmath implementation is different, if we directly remove these decls in builtins.hpp, cmath libdevice test suite will fail, this is why I added the table checking for math functions.
Thanks very much.

@jinge90
Copy link
Contributor Author

jinge90 commented Sep 19, 2023

Can you explain why this is required? This seems strange to me.

With recent changes to SYCL builtin definitions, having conflicting math builtins in the global namespace may cause ambiguity errors for users with using namespace sycl in their code. Where cmath is included we cannot avoid this, but we should at the very least avoid declaring/including them in the SYCL headers.

I'm working on a different approach to this - sycl...aelovikov-intel:llvm:no-complex-include.

Hi, @aelovikov-intel
Some more explanation about intention of this PR:
We need these removed decls in builtins.hpp previously because we have to decorate them with "sycl extern", otherwise sycl sema checking will report error which will block users to call functions provided by libdevice.
Now, we remove these decls and in order to avoid triggering sycl sema checking error report, we give green light for these functions already supported by libdevice.

Thanks very much.

Thanks very much.

@elizabethandrews
Copy link
Contributor

With recent changes to SYCL builtin definitions, having conflicting math builtins in the global namespace may cause ambiguity errors for users with using namespace sycl in their code. Where cmath is included we cannot avoid this, but we should at the very least avoid declaring/including them in the SYCL headers.

Can you elaborate on this please? What were the recent changes which introduced this ambiguity? By conflicting builtins, do you mean the ones in sycl/builtins.hpp and libm?

Can you also answer @AaronBallman's question above? Why isn't the !Callee->getBuiltinID() check in SemaSYCL.cpp catching this case?

Does it make sense to avoid the SYCL_EXTERNAL error for all functions declared in system headers?

@jinge90
Copy link
Contributor Author

jinge90 commented Sep 21, 2023

With recent changes to SYCL builtin definitions, having conflicting math builtins in the global namespace may cause ambiguity errors for users with using namespace sycl in their code. Where cmath is included we cannot avoid this, but we should at the very least avoid declaring/including them in the SYCL headers.

Can you elaborate on this please? What were the recent changes which introduced this ambiguity? By conflicting builtins, do you mean the ones in sycl/builtins.hpp and libm?

Can you also answer @AaronBallman's question above? Why isn't the !Callee->getBuiltinID() check in SemaSYCL.cpp catching this case?

Does it make sense to avoid the SYCL_EXTERNAL error for all functions declared in system headers?

Hi, @elizabethandrews and @AaronBallman
The reason why "!Callee->getBuiltinID()" didn't hit is the test added "-fno-builtin" flag.
With SYCL libdevice support, the math functions can be used in kernel code no matter whether 'fno-builtin' is added or not. We added 'fno-builtin' flag in test to make sure the behavior is unified in both case.
Currently, we can't avoid the SYCL_EXTERNAL error for all functions declared in system headers. Only a small number of functions from system header are supported in sycl kernel via libdevice, most of them are numeric related functions. For other system functions, SYCL sema should report compfail.
Thanks very much.

@jinge90 jinge90 temporarily deployed to WindowsCILock September 21, 2023 10:04 — with GitHub Actions Inactive
aelovikov-intel added a commit to aelovikov-intel/llvm that referenced this pull request Sep 22, 2023
This follows intel#11196 that did the same with <complex> before. We still want to
support `std::` math functions in user code (as part of
`sycl/doc/extensions/supported/C-CXX-StandardLibrary.rst`). Also, often times
STL implements math functions support somewhat like this:

```
extern "C" {
 extern double cos (double __x) noexcept (true);
}
extern "C++"
{
namespace std __attribute__ ((__visibility__ ("default")))
{
 using ::cos;
}
}
```

As such, we still want to let the compiler know which symbols in the global
namespace are known (e.g. `::cos`) and which aren't and need an error ( `SYCL
kernel cannot call an undefined function without SYCL_EXTERNAL attribute`).

One option to implement that is to recognize those functions as builtins in the
front end (which intel#11014 tries to do). Another approach, taken in this PR, is to
keep providing declaration in SYCL headers but only do so when customer included
`<cmath>` explicitly (via `#include_next`, similarly to `<complex>` support
added in intel#11196).
@jinge90 jinge90 temporarily deployed to WindowsCILock September 25, 2023 03:56 — with GitHub Actions Inactive
@AaronBallman
Copy link
Contributor

The reason why "!Callee->getBuiltinID()" didn't hit is the test added "-fno-builtin" flag.

I read this as "there's no bug to be fixed" -- if the user passes -fno-builtin, we should not be giving them any builtins because that's what they expect. What's more, because these are library builtins, the reason a user would pass -fno-builtin in the first place is because they're providing a symbol that would otherwise come from a builtin; so instead of calling the function the user defined, we silently call our own function that probably does mostly the same thing (so the user may not notice the issue for a long time).

aelovikov-intel added a commit that referenced this pull request Sep 25, 2023
This follows #11196 that did the same with <complex> before. We still
want to support `std::` math functions in user code (as part of
`sycl/doc/extensions/supported/C-CXX-StandardLibrary.rst`). Also, often
times STL implements math functions support somewhat like this:

```
extern "C" {
 extern double cos (double __x) noexcept (true);
}
extern "C++"
{
namespace std __attribute__ ((__visibility__ ("default")))
{
 using ::cos;
}
}
```

As such, we still want to let the compiler know which symbols in the
global namespace are known (e.g. `::cos`) and which aren't and need an
error ( `SYCL kernel cannot call an undefined function without
SYCL_EXTERNAL attribute`).

One option to implement that is to recognize those functions as builtins
in the front end (which #11014 tries to do). Another approach, taken in
this PR, is to keep providing declaration in SYCL headers but only do so
when customer included `<cmath>` explicitly (via `#include_next`,
similarly to `<complex>` support added in #11196).
@jinge90
Copy link
Contributor Author

jinge90 commented Sep 26, 2023

The reason why "!Callee->getBuiltinID()" didn't hit is the test added "-fno-builtin" flag.

I read this as "there's no bug to be fixed" -- if the user passes -fno-builtin, we should not be giving them any builtins because that's what they expect. What's more, because these are library builtins, the reason a user would pass -fno-builtin in the first place is because they're providing a symbol that would otherwise come from a builtin; so instead of calling the function the user defined, we silently call our own function that probably does mostly the same thing (so the user may not notice the issue for a long time).

Hi, @AaronBallman
Do you mean if users pass "-fno-builtin", then it means users will provide their own functions that would otherwise come from a builtin and if they intend to call this function in sycl kernel code, they will add "sycl external" to decorate, so sycl sema checking will not report error for this behavior and we don't need to link libdevice implementation with user's device code either.
Thanks very much!

@AaronBallman
Copy link
Contributor

The reason why "!Callee->getBuiltinID()" didn't hit is the test added "-fno-builtin" flag.

I read this as "there's no bug to be fixed" -- if the user passes -fno-builtin, we should not be giving them any builtins because that's what they expect. What's more, because these are library builtins, the reason a user would pass -fno-builtin in the first place is because they're providing a symbol that would otherwise come from a builtin; so instead of calling the function the user defined, we silently call our own function that probably does mostly the same thing (so the user may not notice the issue for a long time).

Hi, @AaronBallman Do you mean if users pass "-fno-builtin", then it means users will provide their own functions that would otherwise come from a builtin and if they intend to call this function in sycl kernel code, they will add "sycl external" to decorate, so sycl sema checking will not report error for this behavior and we don't need to link libdevice implementation with user's device code either. Thanks very much!

I mean that if a user passes -fno-builtin, they expect their function to be called (regardless of host vs kernel). e.g., https://godbolt.org/z/vPbxGnn1Y If we silently ignore the -fno-builtin, then we'd not call the user's function and that would be really hard for the user to figure out why.

@jinge90 jinge90 closed this Oct 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants