Skip to content

[SYCL] Allow relaxed function pointer support in frontend #17274

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

Open
wants to merge 13 commits into
base: sycl
Choose a base branch
from

Conversation

Naghasan
Copy link
Contributor

@Naghasan Naghasan commented Mar 3, 2025

The patch enables the use of function pointer if the function has the indirectly callable attribute or is has a body. If the platform support function pointers, this allow extended support without requiring an interface to migrate / manage code available from the host.

The behaviour is control via an option on the -fsycl-allow-func-ptr flag.

This makes easier to support code ported over from cuda or using this mode #12757

The patch enables the use of function pointer
if the function has the indireclty callable attribute
or is has a body. This makes easier to support code
ported over from cuda.
@Naghasan Naghasan requested review from a team as code owners March 3, 2025 15:12
@Fznamznon Fznamznon requested a review from AlexeySachkov March 5, 2025 12:19
@AlexeySachkov
Copy link
Contributor

The patch enables the use of function pointer if the function has the indirectly callable attribute or is has a body. If the platform support function pointers, this allow extended support without requiring an interface to migrate / manage code available from the host.

How a user could understand if a platform supports function pointers? Is there an aspect they could check, or is there a documentation for that?

The behaviour is control via an option on the -fsycl-allow-func-ptr flag.
This makes easier to support code ported over from cuda or using this mode #12757

Does it imply that the suggested function pointers syntax is the final for SYCL? Because if not, I don't think that it is the best step in simplifying code porting because that code will have to be rewritten anyway. Note that we do not have a language extension specification which would document the attribute

@Naghasan
Copy link
Contributor Author

Naghasan commented Mar 6, 2025

Does it imply that the suggested function pointers syntax is the final for SYCL?

For the use case I'm doing this work for (CUTLASS), yes. When targeting nvidia, they want to keep the code as is. CUDA supports function pointers and so it is used. It is always used with defined function, hence the addition this relaxed mode: because the code will be in the TU, we don't need about a host API to make the code available (unlike VTable, but you know that better than me).

Though your comment made rethink that we shouldn't expose the flag as a driver one, will revert that part.

Note that we do not have a language extension specification which would document the attribute

Well aware of this, but that's intended to be used in pair with -fsycl-cuda-compat. So I'll document this as part of the compat mode once stable enough.

@Naghasan
Copy link
Contributor Author

More comment on this PR ?

@Naghasan
Copy link
Contributor Author

@AlexeySachkov more comments ?

@Naghasan
Copy link
Contributor Author

@intel/llvm-gatekeepers this ready to be merged, failure are due to CI node having issues

@ldrumm
Copy link
Contributor

ldrumm commented Mar 20, 2025

I'd like to see a completed windows build before merging

@Naghasan
Copy link
Contributor Author

D:\github\actions-runner_work\llvm\llvm\src\clang\include\clang/ASTMatchers/ASTMatchersInternal.h(1569): fatal error C1060: compiler is out of heap space

CI isn't working

@ldrumm
Copy link
Contributor

ldrumm commented Mar 20, 2025

I understand CI isn't working,

D:\github\actions-runner_work\llvm\llvm\src\clang\include\clang/ASTMatchers/ASTMatchersInternal.h(1569): fatal error C1060: compiler is out of heap space

CI isn't working

I can see that CI has failed for this run, but that doesn't mean your code isn't broken on Win32 with MSVC. I will merge if another windows runner can build this

@Naghasan
Copy link
Contributor Author

well, still not better, ca1eafa was fine and clang won't build due to heap since this commit 5d20ee7

@Naghasan
Copy link
Contributor Author

same issue with this PR #17552

@sarnex
Copy link
Contributor

sarnex commented Mar 20, 2025

I gave more RAM to the Windows build VMs, so can you try again @Naghasan Thx

@Naghasan
Copy link
Contributor Author

that will clash with #17559, so please hold the merge thanks

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.

7 participants