Skip to content

[Draft] RFC future type and keepalives #2300

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

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

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 <dan.hoeflinger@intel.com>
Signed-off-by: Dan Hoeflinger <dan.hoeflinger@intel.com>
Signed-off-by: Dan Hoeflinger <dan.hoeflinger@intel.com>
Signed-off-by: Dan Hoeflinger <dan.hoeflinger@intel.com>
Signed-off-by: Dan Hoeflinger <dan.hoeflinger@intel.com>
@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 <dan.hoeflinger@intel.com>
Signed-off-by: Dan Hoeflinger <dan.hoeflinger@intel.com>
Signed-off-by: Dan Hoeflinger <dan.hoeflinger@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.

2 participants