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

[oneDPL] oneDPL '__future' class code cleanup and API fix #1962

Closed
wants to merge 2 commits into from

Conversation

MikeDvorskiy
Copy link
Contributor

@MikeDvorskiy MikeDvorskiy commented Dec 9, 2024

A little bit refactoring and class API fix for oneDPL future type.

  1. In case of compiler time parameter set: there was no way to get the second and following parameters from __future instance via __future::get method.
  2. In case of runtime parameter set (array/buffer): there was no way to get the second and following elements from buffer , which is incapsulated into __future instance via __future::get method.
  3. There was a code coupling between following types: __future, 'sycl::buffer', '__result_and_scratch_storage'

The PR fixes the 1,2,3 mentioned issues.

auto
get()
get(std::size_t __idx = 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Everybody who will take a look at this function will ask - what is template param and run-time param here,
Explanation required, I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The bullets 1 and 2 #1962 (comment) explain it.
Yes, probably it make sense to write a comment in the code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Better to understand this from code...

@@ -701,31 +701,42 @@ struct __deferrable_mode
{
};

//A contract for future class: <sycl::event or other event, a value, sycl::buffers..., or __usm_host_or_buffer_storage>
//Impl details: inheritance (private) instead of aggregation for enabling the empty base optimization.
// An overload of __wait_and_get_value for 'sycl::buffer'
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we modify these function without move them outside?

Copy link
Contributor Author

@MikeDvorskiy MikeDvorskiy Dec 9, 2024

Choose a reason for hiding this comment

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

The change also solves a code coupling issue.
There was a code coupling between following types: __future, 'sycl::buffer', '__result_and_scratch_storage'.

Copy link
Contributor

@SergeyKopienko SergeyKopienko Dec 9, 2024

Choose a reason for hiding this comment

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

But why should move them outside of class __future ?
Let's still use them as class-member functions, why not?

@MikeDvorskiy MikeDvorskiy changed the title [oneDPL] future code cleanup and API fix [oneDPL] oneDPL '__future' class code cleanup and API fix Dec 9, 2024
@MikeDvorskiy MikeDvorskiy marked this pull request as draft December 9, 2024 13:58
@MikeDvorskiy MikeDvorskiy force-pushed the dev/mdvorski/future_code_cleanup branch 2 times, most recently from 56968d6 to 065b75d Compare December 9, 2024 16:33
@MikeDvorskiy MikeDvorskiy force-pushed the dev/mdvorski/future_code_cleanup branch from 065b75d to 8232c73 Compare December 9, 2024 16:48
@MikeDvorskiy MikeDvorskiy force-pushed the dev/mdvorski/future_code_cleanup branch from e1e50e3 to 07fd3f8 Compare December 9, 2024 17:02
@MikeDvorskiy MikeDvorskiy marked this pull request as ready for review December 9, 2024 17:07
@MikeDvorskiy
Copy link
Contributor Author

In general, the the change doesn't solve a use case when we need to return/get a couple of values from a future objects.
And there is simpler way - to use, for example, std::pair as T type for __result_and_scratch_storage instead of specifying 2 elements in __result_and_scratch_storage constructor. So, I don't see significant sense with the proposed changes and intend close the PR.

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.

2 participants