Skip to content
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

[SYCL] Annotate virtual calls in device IR #14051

Conversation

AlexeySachkov
Copy link
Contributor

@AlexeySachkov AlexeySachkov commented Jun 5, 2024

This commit is a part of virtual functions extension implementation that is proposed in #10540.

As part of the feature, we would like to achieve both:

  • allow virtual function calls from SYCL kernels that are submitted with the right properties as described in the language extension specification
  • disallow virtual function calls from SYCL kernels that do not use any (or the right) properties to conform with the core SYCL 2020 specification

Implementing such diagnostics will require call graph traversal to understand the origin of the call and that is not what's easy to do in FE. Therefore, the implementation design for virtual functions proposes that such diagnostic is offloaded to a pass. The draft of the analysis pass implementation can be seen in #14141

This commit is a preparations for it: it introduces an attribute to all virtual function calls in device code so they can be easily detected and not confused with plain function pointer calls (for which we don't have a language specification and therefore well-defined behavior yet).

Note that the new approach will relax virtual function call diagnostics, because it will allow non-virtual calls to virtual functions to be performed from kernels. This is in line with discussion in KhronosGroup/SYCL-Docs#565 about relaxing current SYCL 2020 restrictions about virtual functions.

This commit is a part of virtual functions extension implementation that
is proposed in intel#10540.

As part of the feature, we would like to achieve both:
- allow virtual function calls from SYCL kernels that are submitted with
  the right properties as described in the language extension
  specification
- disallow virtual function calls from SYCL kernels that do not use any
  (or the right) properties to conform with the core SYCL 2020
  specification

Implementing such diagnostics will require call graph traversal to
understand the origin of the call and that is not what's easy to do in
FE. Therefore, the implementation design for virtual functions proposes
that such diagnostic is offloaded to a pass.

This commit is a preparations for it: it introduces an attribute to all
virtual function calls in device code so they can be easily detected and
not confused with plain function pointer calls (for which we don't have
a language specification and therefore well-defined behavior yet).
@AlexeySachkov
Copy link
Contributor Author

The only failed test is:

Failed Tests (1):
  Clang :: Driver/sycl-offload-new-driver.c

And it is unrelated to this PR, it was addressed in #14051.

@intel/dpcpp-cfe-reviewers, could you please take a look?

@AlexeySachkov
Copy link
Contributor Author

Tagging @gmlueck here explicitly to review the added test case to check if our virtual calls detection is good enough in context of KhronosGroup/SYCL-Docs#565 and potential SYCL spec changes.

@gmlueck
Copy link
Contributor

gmlueck commented Jul 12, 2024

Tagging @gmlueck here explicitly to review the added test case to check if our virtual calls detection is good enough in context of KhronosGroup/SYCL-Docs#565 and potential SYCL spec changes.

The test cases look good. I agree that the two cases labeled "Strictly speaking, this is a virtual function call, but clang emits it as a direct call" are a little weird, but it seems like both clang and gcc emit these as direct calls. Therefore, I think it is OK if we treat this as a direct call for diagnostic purposes.

@AlexeySachkov AlexeySachkov merged commit 0e24ac5 into intel:sycl Jul 12, 2024
14 checks passed
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.

5 participants