Skip to content

[SYCL][ESIMD] Add new raw_send APIs moving compile time params to template params #10167

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 3 commits into from
Jul 31, 2023

Conversation

sarnex
Copy link
Contributor

@sarnex sarnex commented Jun 30, 2023

The arguments moved from runtime params to template params need to be compile time constant.

Often they end up as compile time constant after constant folding, but not in all cases such as -O0, and if they are not constant it will lead to an IGC error.

Add new APIs to enforce this requirement.

I also updated three LSC APIs to use the new version, but these were already passing in constexpr arguments.

@sarnex sarnex temporarily deployed to aws June 30, 2023 19:37 — with GitHub Actions Inactive
@sarnex
Copy link
Contributor Author

sarnex commented Jun 30, 2023

I'll be on vacation until July 26th, I'll address any feedback then. Thanks!

@sarnex sarnex marked this pull request as ready for review June 30, 2023 21:00
@sarnex sarnex requested a review from a team as a code owner June 30, 2023 21:00
@sarnex sarnex temporarily deployed to aws June 30, 2023 22:10 — with GitHub Actions Inactive
@v-klochkov
Copy link
Contributor

I suggest using a simpler approach that does not require adding new _esimd* internal intrinsics.
We have slm_init() and slm_init(Size). Both use the same internal intrinsic and that is just enough for compiler to understand that the arguments are constant:

template <uint32_t SLMSize> __ESIMD_API void slm_init() {
  __esimd_slm_init(SLMSize);
}

@sarnex
Copy link
Contributor Author

sarnex commented Jul 27, 2023

Good idea, I didn't consider using template params into arguments to the intrinsic that accepts runtime params, thanks, let me try!

sarnex added 2 commits July 27, 2023 10:42
… template arguments

The arguments moved from runtime params to template params need to be compile time constant.

Often they end up as compile time constant after constant folding, but not in all cases such as -O0,
which will lead to an IGC error.

Add new APIs to enforce this requirement.

I also updated three LSC APIs to use the new version, but these were already passing in constexpr arguments.

Signed-off-by: Sarnie, Nick <[email protected]>
@sarnex sarnex temporarily deployed to aws July 27, 2023 19:13 — with GitHub Actions Inactive
@sarnex
Copy link
Contributor Author

sarnex commented Jul 27, 2023

BTW I manually verified the issue is fixed with the latest version of this change if you use the new API.

@sarnex sarnex temporarily deployed to aws July 27, 2023 19:52 — with GitHub Actions Inactive
Copy link
Contributor

@turinevgeny turinevgeny left a comment

Choose a reason for hiding this comment

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

LGTM, a small nit.

@sarnex sarnex temporarily deployed to aws July 28, 2023 18:40 — with GitHub Actions Inactive
@sarnex sarnex temporarily deployed to aws July 28, 2023 19:18 — with GitHub Actions Inactive
@v-klochkov v-klochkov merged commit 1a5e532 into intel:sycl Jul 31, 2023
mdtoguchi pushed a commit to mdtoguchi/llvm that referenced this pull request Oct 18, 2023
…plate params (intel#10167)

The arguments moved from runtime params to template params need to be
compile time constant.

Often they end up as compile time constant after constant folding, but
not in all cases such as -O0, and if they are not constant it will lead
to an IGC error.

Add new APIs to enforce this requirement.

I also updated three LSC APIs to use the new version, but these were
already passing in constexpr arguments.

---------

Signed-off-by: Sarnie, Nick <[email protected]>
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.

3 participants