Skip to content

Conversation

danhoeflinger
Copy link
Contributor

This is a work in progress draft.

This RFC outlines some issues with our current __future and __result_and_scratch_storage, and start to lay out a path to toward improvements.

Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
@SergeyKopienko
Copy link
Contributor

My proposal:

  • remove __future usage on all internal levels at all and use it only in async algorithm implementations (idea of @akukanov) - this approach implemented in the PR Remove the usages of __future on internal layers #2261
  • extend functional of __result_and_scratch_storage to keep different types of result and type of temporary data.
    Probably also make sense to implement void result type without memory allocation for it in this case.

@danhoeflinger
Copy link
Contributor Author

My proposal:

Do you think this PR / proposal resolves the issue of matching return types for runtime branched different implementations of the same algorithm? Or do you see this as a step on the path toward a fix?

  • extend functional of __result_and_scratch_storage to keep different types of result and type of temporary data.
    Probably also make sense to implement void result type without memory allocation for it in this case.

To me, this seems in conflict with the goal of creating space in between real result / return values and keepalive values. I really don't see the value in adding features to the combined type instead of separating the two.

@SergeyKopienko
Copy link
Contributor

Do you think this PR / proposal resolves the issue of matching return types for runtime branched different implementations of the same algorithm? Or do you see this as a step on the path toward a fix?

  • I think this problem is potential, but it's absent right now. At least, for result data types. Also independently of used staff we may to think about alignment of returns data types. I believe this PR remove extra thing like future from the code where it doesn't really required at all.

To me, this seems in conflict with the goal of creating space in between real result / return values and keepalive values. I really don't see the value in adding features to the combined type instead of separating the two.

  • From my point of view, probably it's only one implementation options - to separate results container and temporary data. I mean it may be not the goal (to separate them) but only implementation detail.

In any case I think we may have different opinions about some things and we may discuss about them on the meeting.

@danhoeflinger
Copy link
Contributor Author

@SergeyKopienko I'll try to incorporate your proposals into the RFC today as options.

@danhoeflinger
Copy link
Contributor Author

  • I think this problem is potential, but it's absent right now. At least, for result data types. Also independently of used staff we may to think about alignment of returns data types. I believe this PR remove extra thing like future from the code where it doesn't really required at all.

I think this is actually a real problem we have worked around in the past (reduce_then_scan vs single_workgroup_scan vs scan_then_propagate). I have been experiencing this even more acutely recently with the set algorithms, which in some cases may end with a merge operation, and in another may end with a scan operation. I will try to add some examples to the RFC.

Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
@SergeyKopienko
Copy link
Contributor

I think we may try to isolate temporary data on sycl-submitter's level.

Probably we can implement this approach like in variant (c) in RFC:

  • create special final host task in each sycl-submitter which has temporary data;
  • finalize their temporary data on this special final host task.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants