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.

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

Allow any manifestly constant-evaluated expression and conversion in device function #388

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Allow any manifestly constant-evaluated expression and conversion in device function #388

wants to merge 2 commits into from

Conversation

keryell
Copy link
Member

@keryell keryell commented Mar 22, 2023

This should address #267

@keryell keryell mentioned this pull request Mar 22, 2023
Open
@TApplencourt
Copy link
Contributor

Look good enough to get us started :). In theory, if recursion is tail-recurvice if you be allowed to (as it will not need a call stack on the GPU)

@keryell
Copy link
Member Author

keryell commented Apr 13, 2023

Look good enough to get us started :). In theory, if recursion is tail-recurvice if you be allowed to (as it will not need a call stack on the GPU)

You assume that the compiler is smart enough to replace a tail-recursion by a loop. What if -O0? It looks like a QoI problem to be included in the specification.

@Naghasan
Copy link
Member

I agree with Ronan, What if -O0? and what happens in debug as well (you may not want optimization there as well)

@TApplencourt
Copy link
Contributor

TApplencourt commented Apr 13, 2023 •
edited
Loading

https://reviews.llvm.org/D99517, LLVM have now a pragma that should force it for example. But I see your point that it's "just an optimization". Same as constexpr, so if by constant evaluation. we mean manifestly constant evaluated, it looks good to me.

The goal is only to ensure that the generated accelerator backend code should not have recursion; how we achieve this goal can be multiple.

@tomdeakin
Copy link
Contributor

WG: Better approach is to add sentence about constexpr, and recursion example is a non-normative note.

@keryell keryell changed the title Allow recursion in kernels in the context of constant evaluation Allow any manifestly constant evaluation in kernel context May 4, 2023
@keryell keryell marked this pull request as draft May 11, 2023 15:07
@keryell
Waiver on manifestly constant-evaluation in kernel code
bdc8fea 
In the case of a manifestly constant-evaluated expression or
conversion, any code accepted by the C++ standard in this case is also
accepted in SYCL device function.
@keryell keryell force-pushed the ronan/SYCL-2020/constant-evaluation-recursion branch from ef19c48 to bdc8fea Compare May 17, 2023 23:35
@keryell keryell changed the title Allow any manifestly constant evaluation in kernel context Allow any manifestly constant-evaluated expression and conversion in kernel context May 17, 2023
@keryell keryell changed the title Allow any manifestly constant-evaluated expression and conversion in kernel context Allow any manifestly constant-evaluated expression and conversion in device function May 17, 2023
@keryell
Copy link
Member Author

keryell commented May 17, 2023

This has been generalized to address #379

@keryell keryell marked this pull request as ready for review May 17, 2023 23:38
@fraggamuffin
Copy link

bug fix or NEXT or KHR?
NEXT or KHR?

@keryell keryell marked this pull request as draft May 31, 2023 23:58
@keryell
Copy link
Member Author

keryell commented May 31, 2023

Changing to a draft to wait for SYCL Next.

@tomdeakin
Copy link
Contributor

Please open MR on GitLab. Thanks!

@keryell
Copy link
Member Author

keryell commented Feb 8, 2024

I have rebased this PR for SYCL Next as internal https://gitlab.khronos.org/sycl/Specification/-/merge_requests/740

@keryell keryell marked this pull request as ready for review February 8, 2024 17:54
@keryell
Copy link
Member Author

keryell commented Feb 8, 2024 •
edited
Loading

Just removing the Draft status to consider this to be closed during next meeting and look at GitLab.

@keryell keryell mentioned this pull request Mar 21, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TApplencourt TApplencourt Awaiting requested review from TApplencourt

Assignees
No one assigned
Labels
Postpone to next SYCL version
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@keryell @TApplencourt @Naghasan @tomdeakin @fraggamuffin