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.

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

[SYCL] Do not use handler::addAccessorReq() #16111

Conversation

Alexandr-Konovalov
Copy link
Contributor

@Alexandr-Konovalov Alexandr-Konovalov commented Nov 18, 2024

This function might be useful only when an accessor is connected with different
CG that was used during a construction of the accessor. Currently it’s helpful
only in single case of reduction, and in this case the accessor can be connected
to a right CG in the constructor. Modify the use case and remove all calls to
addAccessorReq().

Constructor of accessors add them to MRequirements and MAccStorage
of the associated handler, so do not add duplicates here if same
handler was used during construction.
@Alexandr-Konovalov Alexandr-Konovalov force-pushed the Alexandr-Konovalov/duplicated-Requirements branch from 4ff4a38 to 28711a7 Compare November 20, 2024 10:59
@Alexandr-Konovalov Alexandr-Konovalov changed the title [SYCL] Do not call handler::addAccessorReq() [SYCL] Check for duplicates in handler::addAccessorReq() Nov 20, 2024
@sergey-semenov sergey-semenov self-requested a review November 20, 2024 17:20
@Alexandr-Konovalov Alexandr-Konovalov changed the title [SYCL] Check for duplicates in handler::addAccessorReq() [SYCL] Do not use handler::addAccessorReq() Nov 28, 2024
@Alexandr-Konovalov Alexandr-Konovalov marked this pull request as ready for review November 29, 2024 06:53
@Alexandr-Konovalov Alexandr-Konovalov requested a review from a team as a code owner November 29, 2024 06:53
@Alexandr-Konovalov
Copy link
Contributor Author

@intel/llvm-gatekeepers Could you please look at the PR?

@steffenlarsen
Copy link
Contributor

My biggest worry about this patch is that we no longer keep accessors alive in the accessor storage of the command-group handler for many of these paths. Is this safe? Are we confident there are no scenarios that need them alive? I can see the graph copies the storage, @intel/sycl-graphs-reviewers.

@steffenlarsen steffenlarsen requested review from a team and EwanC December 10, 2024 07:34
@Bensuo
Copy link
Contributor

Bensuo commented Dec 10, 2024

My biggest worry about this patch is that we no longer keep accessors alive in the accessor storage of the command-group handler for many of these paths. Is this safe? Are we confident there are no scenarios that need them alive? I can see the graph copies the storage, @intel/sycl-graphs-reviewers.

I think this change should be fine for us in general if I'm understanding it correctly. Accessors are by default stored when associated with a handler (either through the ctor taking a handler or handler::require()) via this code. So outside of this niche usecase where the accessors needed to be associated with an additional handler I think this would have just been creating duplicate requirements for calls that took these paths.

We don't currently support reductions in graphs but I don't think this change makes much difference to the challenges of us implementing support for them so it seems fine in that regard also.

@steffenlarsen
Copy link
Contributor

Thanks, @Bensuo! In that case, I forfeit my concerns. Let's roll with it!

@steffenlarsen steffenlarsen merged commit c16775d into intel:sycl Dec 10, 2024
14 checks passed
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.

4 participants