Skip to content

[SYCL] Don't define SYCL_EXTERNAL if -fno-sycl-rdc is passed #8537

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.

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

Merged
merged 8 commits into from
Mar 8, 2023

Conversation

sarnex
Copy link
Contributor

@sarnex sarnex commented Mar 3, 2023

Please review this commit by commit because one of the commits affects a ton of files. Thanks!

The SYCL spec allows us to declare we don't support device function calls outside of the current translation unit by not defining the SYCL_EXTERNAL macro.
With the -fno-sycl-rdc option, this is exactly that case: we do not support calls outside of the TU, so don't define the macro.

However, we define and use the SYCL_EXTERNAL in the SYCL headers also, so instead add __DPCPP_SYCL_EXTERNAL and use that. It is defined to SYCL_EXTERNAL if SYCL_EXTERNAL is defined, and otherwise __attribute__((sycl_device)) for device code or empty for host. This is fine because none of the uses here actually end up as cross-TU calls, it's either linked in from the device libraries or replaced by the SPIR-V translator to some SPIR-V intrinsic.

I added a test in sycl/test/basic_tests/macros_no_rdc.cpp to make sure nobody uses SYCL_EXTERNAL directly in the SYCL headers in the future.

(UPDATE: This part has been removed) However, with the macro being undefined, if a user tries to use it, they just get the classic unknown type name 'SYCL_EXTERNAL' which is not super helpful. I added some really gross code to give a better error message, but I'd like to hear from the CFE folks if there's a better way to do it, and if not, if it's so gross we should just drop it and rely on the normal error.

Relevant discussion: #8479

@sarnex sarnex temporarily deployed to aws March 3, 2023 21:24 — with GitHub Actions Inactive
@sarnex sarnex temporarily deployed to aws March 3, 2023 22:06 — with GitHub Actions Inactive
@sarnex
Copy link
Contributor Author

sarnex commented Mar 6, 2023

 Failed Tests (1):
    SYCL :: SubGroup/shuffle_fp64.cpp

unrelated

@sarnex sarnex marked this pull request as ready for review March 6, 2023 14:50
@sarnex sarnex requested review from a team and rolandschulz as code owners March 6, 2023 14:50
@sarnex sarnex requested review from steffenlarsen and bader March 6, 2023 14:50
Copy link
Contributor

@Fznamznon Fznamznon left a comment

Choose a reason for hiding this comment

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

Since SYCL spec allows to not provide the macro, I wonder if it should be up to user to check whether the macro is defined by an implementation before using it?

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.

Small question for my own education, but the runtime changes LGTM.

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.

FE changes LGTM

@sarnex sarnex temporarily deployed to aws March 7, 2023 18:12 — with GitHub Actions Inactive
@sarnex
Copy link
Contributor Author

sarnex commented Mar 8, 2023

@v-klochkov Mind reviewing this? This is an NFC from ESIMD POV, just header macro name changes. Thanks!

@sarnex
Copy link
Contributor Author

sarnex commented Mar 8, 2023

********************
Timed Out Tests (3):
  SYCL :: DeviceLib/assert-aot.cpp
  SYCL :: DeviceLib/assert.cpp
  SYCL :: KernelFusion/pointer_arg_function.cpp

unrelated

@sarnex
Copy link
Contributor Author

sarnex commented Mar 8, 2023

@bader @intel/llvm-gatekeepers Hey all, can we merge? CI failures unrelated. Thanks!

@AlexeySachkov
Copy link
Contributor

Since SYCL spec allows to not provide the macro, I wonder if it should be up to user to check whether the macro is defined by an implementation before using it?

It is indeed up to user to check if the macro is defined before using it

Signed-off-by: Sarnie, Nick <[email protected]>
@sarnex sarnex temporarily deployed to aws March 8, 2023 19:48 — with GitHub Actions Inactive
@sarnex sarnex temporarily deployed to aws March 8, 2023 20:45 — with GitHub Actions Inactive
@bader bader merged commit 9912ac2 into intel:sycl Mar 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.

10 participants