Skip to content

[SYCL] Don't include <complex> from <sycl/sycl.hpp> #11196

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 12 commits into from
Sep 22, 2023
Merged

[SYCL] Don't include <complex> from <sycl/sycl.hpp> #11196

merged 12 commits into from
Sep 22, 2023

Conversation

aelovikov-intel
Copy link
Contributor

@aelovikov-intel aelovikov-intel commented Sep 15, 2023 •
edited
Loading

This addresses a part of a bigger issue of polluting the global namespace with math functions by implicitly including <cmath> from sycl headers. Internally, <complex> includes <cmath> so we shouldn't be including it as well, which this patch implements.

I'm doing it by providing our own version of that does #include_next <complex> and also provides needed functions/template specializations to support std::complex-related functionality whenever user program includes on its own. System include path for this is only provided in SYCL mode, so non-SYCL compilation flow is unaffected.

Unfortunately, MSVC doesn't have have #include_next so we use a hack to emulate it - via #include <../include/complex>.

@aelovikov-intel
[SYCL] Don't include <complex> from <sycl/sycl.hpp>
b7e5f95 
This addresses a part of a bigger issue of polluting the global
namespace with math functions by implicitly including <cmath> from sycl
headers. Internally, <complex> includes <cmath> so we shouldn't be
including it as well, which this patch implements.

I'm doing it by providing our own version of <complex> that does
`#include_next <complex>` and also provides needed functions/template
specializations to support `std::complex`-related functionality whenever
user program includes <complex> on its own. System include path for this
is only provided in SYCL mode so non-SYCL compilation flow is unaffected.
@aelovikov-intel aelovikov-intel requested review from a team as code owners September 15, 2023 20:32
bader
bader reviewed Sep 15, 2023
View reviewed changes
mdtoguchi
mdtoguchi reviewed Sep 15, 2023
View reviewed changes
aelovikov-intel
aelovikov-intel commented Sep 15, 2023
View reviewed changes
clang/test/Driver/sycl-offload.c
// CHECK-HEADER-DIR: clang{{.*}} "-fsycl-is-host"{{.*}} "-internal-isystem" "{{.*}}bin{{[/\\]+}}..{{[/\\]+}}include{{[/\\]+}}sycl" "-internal-isystem" "{{.*}}bin{{[/\\]+}}..{{[/\\]+}}include"{{.*}}
// CHECK-HEADER-DIR: clang{{.*}} "-fsycl-is-device"
// CHECK-HEADER-DIR-SAME: "-internal-isystem" "[[ROOT:[^"]*]]bin{{[/\\]+}}..{{[/\\]+}}include{{[/\\]+}}sycl"
// CHECK-HEADER-DIR-NOT: -internal-isystem
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previous version didn't catch cases where we had extra includes between the ones we match (due to {{.*}}bin greedy regex). This update is both catching that and doesn't require extra long lines making it more readable.

mdtoguchi
mdtoguchi approved these changes Sep 15, 2023
View reviewed changes
Copy link
Contributor

@mdtoguchi mdtoguchi left a comment

Choose a reason for hiding this comment

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

OK for driver

bader
bader reviewed Sep 15, 2023
View reviewed changes
bader
bader reviewed Sep 15, 2023
View reviewed changes
bader
bader reviewed Sep 16, 2023
View reviewed changes
keryell
keryell reviewed Sep 16, 2023
View reviewed changes
@aelovikov-intel aelovikov-intel temporarily deployed to WindowsCILock September 20, 2023 20:36 — with GitHub Actions Inactive
@aelovikov-intel aelovikov-intel temporarily deployed to WindowsCILock September 20, 2023 21:30 — with GitHub Actions Inactive
@aelovikov-intel
Copy link
Contributor Author

@cperkinsintel , @intel/llvm-reviewers-runtime ping.

@cperkinsintel
Copy link
Contributor

So if a user wants to use complex numbers, they need to #include <complex> themselves, and do so before #include <sycl/sycl.hpp>, correct? Or am I misunderstanding how this works?

@aelovikov-intel
Copy link
Contributor Author

So if a user wants to use complex numbers, they need to #include <complex> themselves, and do so before #include <sycl/sycl.hpp>, correct? Or am I misunderstanding how this works?

Both before/after would work - see https://github.com/intel/llvm/pull/11196/files#diff-d8871abe2a3a33ba61392bd95ccf6f1caab18118964cd0814868b73afeae4af0.

@aelovikov-intel aelovikov-intel marked this pull request as draft September 21, 2023 19:38
@aelovikov-intel aelovikov-intel temporarily deployed to WindowsCILock September 21, 2023 20:28 — with GitHub Actions Inactive
@aelovikov-intel aelovikov-intel temporarily deployed to WindowsCILock September 21, 2023 21:26 — with GitHub Actions Inactive
@aelovikov-intel aelovikov-intel temporarily deployed to WindowsCILock September 21, 2023 21:47 — with GitHub Actions Inactive
@aelovikov-intel aelovikov-intel marked this pull request as ready for review September 21, 2023 21:49
cperkinsintel
cperkinsintel approved these changes Sep 21, 2023
View reviewed changes
Copy link
Contributor

@cperkinsintel cperkinsintel left a comment

Choose a reason for hiding this comment

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

ship it

@aelovikov-intel aelovikov-intel temporarily deployed to WindowsCILock September 21, 2023 23:02 — with GitHub Actions Inactive
@steffenlarsen steffenlarsen merged commit 6ed0ab8 into intel:sycl Sep 22, 2023
@aelovikov-intel aelovikov-intel deleted the no-complex branch September 22, 2023 14:36
@aelovikov-intel
Copy link
Contributor Author

@mdtoguchi , there was small driver change after your approval to support -fsycl-host-compiler. Can you take a look post-commit, please?

@aelovikov-intel aelovikov-intel mentioned this pull request Sep 22, 2023
Merged
aelovikov-intel added a commit to aelovikov-intel/llvm that referenced this pull request Sep 22, 2023
@aelovikov-intel
[SYCL] Don't include <cmath> from <sycl/sycl.hpp>
a174ee0 
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).
aelovikov-intel added a commit that referenced this pull request Sep 25, 2023
@aelovikov-intel
[SYCL] Don't include <cmath> from <sycl/sycl.hpp> (#11274)
9bbbc77 
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).
@aelovikov-intel aelovikov-intel mentioned this pull request Sep 27, 2023
Merged
aelovikov-intel added a commit to aelovikov-intel/llvm that referenced this pull request Sep 27, 2023
@aelovikov-intel
[SYCL] Include <complex>/<cmath> from sycl.hpp, unless SYCL2020_CONFO…
d5693d0 
…RMANT_APIS

intel#11274 and
intel#11196 could break customers code that
relied on implicit inclusion of those headers. Don't break it before the
next ABI breaking window.
steffenlarsen pushed a commit that referenced this pull request Sep 28, 2023
@aelovikov-intel
[SYCL] Include <complex>/<cmath> from sycl.hpp, unless SYCL2020_CONFO…
d22c9c5 
…RMANT_APIS (#11326)

#11274 and
#11196 could break customers code that
relied on implicit inclusion of those headers. Don't break it before the
next ABI breaking window.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bader bader bader left review comments

@keryell keryell keryell left review comments

@cperkinsintel cperkinsintel cperkinsintel approved these changes

@mdtoguchi mdtoguchi Awaiting requested review from mdtoguchi

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@aelovikov-intel @cperkinsintel @bader @keryell @mdtoguchi @steffenlarsen