Skip to content

[SYCL] Fix use_root_sync handling #17739

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 6 commits into from
Apr 4, 2025
Merged

[SYCL] Fix use_root_sync handling #17739

merged 6 commits into from
Apr 4, 2025

Conversation

HPS-1
Copy link
Contributor

@HPS-1 HPS-1 commented Mar 31, 2025

Address issue #16451 , where property use_root_sync is not processed properly. Also revised sycl/test-e2e/GroupAlgorithm/root_group.cpp to not use the deprecated version of parallel_for. (Which was previously blocked by this issue about use_root_sync).

Also here's some explanation for the change in handler.hpp:
This is where the previous code doesn't handle use_root_sync correctly: processLaunchProperties will be called twice, first for the property list returned by the kernel functor's get(properties_tag) method, and then for Props that is passed in as a parameter to parallel_for. Therefore, if the get(properties_tag) method specifies use_root_sync and Props is empty or doesn't contain use_root_sync, what will be done is:

  • first, the property list returned by the kernel functor's get(properties_tag) method get processed. And since it contains use_root_sync, setKernelIsCooperative(true) is called;
  • then, the property list Props that is passed in as a parameter to parallel_for get processed. And since it doesn't contain use_root_sync (actually for the non-deprecated variants of parallel_for, Props should always be an empty property list), setKernelIsCooperative(**false**) is called
    And thus in the end the MKernelIsCooperative flag will be set to false, while it actually should be true. Revising the code like this solve the problem.

Also MKernelIsCooperative is false by default, so we don't need to worry if setKernelIsCooperative is not called.

Signed-off-by: Hu, Peisen <peisen.hu@intel.com>
HPS-1 added 2 commits March 31, 2025 06:44
Signed-off-by: Hu, Peisen <peisen.hu@intel.com>
Signed-off-by: Hu, Peisen <peisen.hu@intel.com>
Signed-off-by: Hu, Peisen <peisen.hu@intel.com>
Signed-off-by: Hu, Peisen <peisen.hu@intel.com>
@HPS-1 HPS-1 temporarily deployed to WindowsCILock March 31, 2025 16:59 — with GitHub Actions Inactive
@HPS-1 HPS-1 temporarily deployed to WindowsCILock March 31, 2025 17:12 — with GitHub Actions Inactive
@HPS-1 HPS-1 temporarily deployed to WindowsCILock March 31, 2025 17:12 — with GitHub Actions Inactive
Signed-off-by: Hu, Peisen <peisen.hu@intel.com>
@HPS-1 HPS-1 temporarily deployed to WindowsCILock March 31, 2025 19:50 — with GitHub Actions Inactive
@HPS-1 HPS-1 temporarily deployed to WindowsCILock March 31, 2025 20:41 — with GitHub Actions Inactive
@HPS-1 HPS-1 temporarily deployed to WindowsCILock March 31, 2025 20:41 — with GitHub Actions Inactive
@HPS-1 HPS-1 marked this pull request as ready for review April 1, 2025 09:59
@HPS-1 HPS-1 requested a review from a team as a code owner April 1, 2025 09:59
@HPS-1 HPS-1 requested a review from KseniyaTikhomirova April 1, 2025 09:59
Copy link
Contributor

@KseniyaTikhomirova KseniyaTikhomirova left a comment

Choose a reason for hiding this comment

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

LGTM

@HPS-1
Copy link
Contributor Author

HPS-1 commented Apr 3, 2025

@intel/llvm-gatekeepers Please merge this, thanks!

@uditagarwal97
Copy link
Contributor

Hi @HPS-1 ,
Github says : This commit will be authored by 65581261+HPS-1@users.noreply.github.com.
Can you change your GH profile settings to make your intel email public?
Refer https://docs.github.com/en/account-and-profile/setting-up-and-managing-your-personal-account-on-github/managing-email-preferences/setting-your-commit-email-address

Also, @sarnex private email workflow isn't working.

@uditagarwal97
Copy link
Contributor

Also, should #16451 be resolved after this PR? If so, please consider linking this issue to your PR. Here's how to do so: https://docs.github.com/en/issues/tracking-your-work-with-issues/using-issues/linking-a-pull-request-to-an-issue

@HPS-1
Copy link
Contributor Author

HPS-1 commented Apr 3, 2025

Hi @HPS-1 , Github says : This commit will be authored by 65581261+HPS-1@users.noreply.github.com. Can you change your GH profile settings to make your intel email public? Refer https://docs.github.com/en/account-and-profile/setting-up-and-managing-your-personal-account-on-github/managing-email-preferences/setting-your-commit-email-address

Also, @sarnex private email workflow isn't working.

Hi @uditagarwal97 ! I've made my emails public, can you try merging again? Thanks!

@HPS-1
Copy link
Contributor Author

HPS-1 commented Apr 3, 2025

Also, should #16451 be resolved after this PR? If so, please consider linking this issue to your PR. Here's how to do so: https://docs.github.com/en/issues/tracking-your-work-with-issues/using-issues/linking-a-pull-request-to-an-issue

Linked!

@HPS-1
Copy link
Contributor Author

HPS-1 commented Apr 4, 2025

@intel/llvm-gatekeepers Can you try merge this again? Thanks!

@martygrant martygrant merged commit c1094a0 into intel:sycl Apr 4, 2025
30 of 35 checks passed
KornevNikita pushed a commit that referenced this pull request May 27, 2025
Address issue #16451 , where
property `use_root_sync` is not processed properly. Also revised
`sycl/test-e2e/GroupAlgorithm/root_group.cpp` to not use the deprecated
version of `parallel_for`. (Which was previously blocked by this issue
about `use_root_sync`).

Also here's some explanation for the change in `handler.hpp`:
This is where the previous code doesn't handle `use_root_sync`
correctly: `processLaunchProperties` will be called twice, first for the
property list returned by the kernel functor's `get(properties_tag)`
method, and then for `Props` that is passed in as a parameter to
`parallel_for`. Therefore, if the `get(properties_tag)` method specifies
`use_root_sync` and `Props` is empty or doesn't contain `use_root_sync`,
what will be done is:

- first, the property list returned by the kernel functor's
`get(properties_tag)` method get processed. And since it contains
`use_root_sync`, `setKernelIsCooperative(true)` is called;
- then, the property list `Props` that is passed in as a parameter to
`parallel_for` get processed. And since it doesn't contain
`use_root_sync` (actually for the non-deprecated variants of
`parallel_for`, `Props` should always be an empty property list),
`setKernelIsCooperative(**false**)` is called
And thus in the end the `MKernelIsCooperative` flag will be set to
false, while it actually should be true. Revising the code like this
solve the problem.

Also `MKernelIsCooperative` is false by default, so we don't need to
worry if `setKernelIsCooperative` is not called.

---------

Signed-off-by: Hu, Peisen <peisen.hu@intel.com>
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.

Kernel property use_root_sync not specified correctly for kernel functors
5 participants