Skip to content

[SYCL][ESIMD] Implement compile-time getNextPowerOf2 w/o recursion. #2133

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 1 commit into from
Jul 22, 2020

Conversation

kbobrovs
Copy link
Contributor

Device code is restricted not to allow recursion.

Signed-off-by: Konstantin S Bobrovsky [email protected]

@kbobrovs kbobrovs requested a review from DenisBakhvalov July 17, 2020 06:56
@kbobrovs kbobrovs requested a review from a team as a code owner July 17, 2020 06:56
@kbobrovs kbobrovs requested a review from againull July 17, 2020 06:56
@kbobrovs
Copy link
Contributor Author

Some more info on the problem is here #2104. Simply allowing recursion for constexpr functions does not work as they can be called from non-constexpr context.

@kbobrovs kbobrovs added the esimd Explicit SIMD feature label Jul 17, 2020
Copy link
Contributor

@keryell keryell left a comment

Choose a reason for hiding this comment

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

I do not understand want you are doing here.
You had a recursion before, you have a recursion after...
Are you looking at something like https://github.com/triSYCL/triSYCL/blob/master/include/triSYCL/pipe.hpp#L55 ?

@kbobrovs
Copy link
Contributor Author

kbobrovs commented Jul 20, 2020

You had a recursion before, you have a recursion after...

I had a real recursion before, where compiler would leave the recursive function in the IR if is called from non-constexpr context.

In the version "after" the expansion is guaranteed to happen at compile-time, and IR is guaranteed not to have recursive function.

Copy link
Contributor

@againull againull left a comment

Choose a reason for hiding this comment

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

If I understand it correctly, log2 also have real recursion and should be rewritten too?
Will it be done separately?

@kbobrovs
Copy link
Contributor Author

If I understand it correctly, log2 also have real recursion and should be rewritten too?
Will it be done separately?

good catch. It is not used now, hence does not cause compilation errors. Should be removed for now - will do.

@kbobrovs
Copy link
Contributor Author

actually it is used in esimd_reduce, so will re-implement

Can't have real recursion, even if constexpr - SYCL device compiler does
not allow that according to the spec.

Signed-off-by: Konstantin S Bobrovsky <[email protected]>
@kbobrovs
Copy link
Contributor Author

Had to force-push to provide good commit message. Hope this is OK, since the patch is small.

@againull againull self-requested a review July 21, 2020 17:10
@kbobrovs
Copy link
Contributor Author

@keryell, are you OK with the patch?

Copy link
Contributor

@keryell keryell left a comment

Choose a reason for hiding this comment

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

This is a good trade-off while waiting for simplifying all of this with C++20 some day.

@bader bader merged commit 71e612a into intel:sycl Jul 22, 2020
@kbobrovs kbobrovs deleted the esimd1-np2 branch July 30, 2020 12:29
jsji pushed a commit that referenced this pull request Aug 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
esimd Explicit SIMD feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants