Skip to content

Initial draft of data flow pipes extension proposal, and move files into pipes subfolder #635

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Nov 14, 2019
Merged

Initial draft of data flow pipes extension proposal, and move files into pipes subfolder #635

merged 5 commits into from
Nov 14, 2019

Conversation

mkinsner
Copy link

No description provided.

@bader bader self-assigned this Sep 12, 2019
@bader bader requested review from asavonic and MrSidims September 12, 2019 20:16
@mkinsner mkinsner mentioned this pull request Sep 13, 2019
Merged
@keryell
Copy link
Contributor

keryell commented Sep 17, 2019

There was also #318 related to this.

keryell
keryell reviewed Sep 17, 2019
View reviewed changes
Copy link
Contributor

@keryell keryell left a comment

Choose a reason for hiding this comment

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

Great proposal!
Here is my review on the first half of the proposal.

keryell
keryell suggested changes Sep 18, 2019
View reviewed changes
Copy link
Contributor

@keryell keryell left a comment

Choose a reason for hiding this comment

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

Thank you for the great proposal.
That looks good on my side, just with a few typos to fix.
The most important point for me is to stream-line the map/unmap API.

@bader
Copy link
Contributor

bader commented Nov 7, 2019

@mkinsner, please, address comments from @keryell.

Michael Kinsner added 2 commits November 13, 2019 10:30
Initial draft of data flow pipes extension proposal, and move files i…
d0817f6 
…nto pipes subfolder.

Signed-off-by: Michael Kinsner <michael.kinsner@intel.com>
Const ref to data in write methods, as suggested in intel#292 (comment)
589224c 
Signed-off-by: Michael Kinsner <michael.kinsner@intel.com>
Bump to revision 2, incorporating feedback
61f0ee7 
Signed-off-by: Michael Kinsner <michael.kinsner@intel.com>
@mkinsner mkinsner mentioned this pull request Nov 13, 2019
Open
@mkinsner
Copy link
Author

Thank you for the great proposal.
That looks good on my side, just with a few typos to fix.
The most important point for me is to stream-line the map/unmap API.

Thanks for the review and feedback @keryell ! I've incorporated almost all of it. Three larger items are tracked for the future in: #832

@mkinsner
Copy link
Author

@bader I believe that this extension revision is ready to merge.

@bader
Copy link
Contributor

bader commented Nov 13, 2019

@bader I believe that this extension revision is ready to merge.

Great! Thank you!
Are you okay to wait for @keryell approve before the merge?

@mkinsner
Copy link
Author

@bader I believe that this extension revision is ready to merge.

Great! Thank you!
Are you okay to wait for @keryell approve before the merge?

I'd prefer to merge this week. We can wait a day or two to see whether @keryell is online and has bandwidth to approve.

keryell
keryell suggested changes Nov 14, 2019
View reviewed changes
Copy link
Contributor

@keryell keryell left a comment

Choose a reason for hiding this comment

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

Just a small typo.

Fix typo in boost::hana::type
5058e73 
Signed-off-by: Michael Kinsner <michael.kinsner@intel.com>
keryell
keryell previously approved these changes Nov 14, 2019
View reviewed changes
Copy link
Contributor

@keryell keryell left a comment

Choose a reason for hiding this comment

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

That looks good.

@MrSidims
Copy link
Contributor

Just one nit:
I think that SYCLPipesUsage.rst can be removed at all.

Remove doc that is now redundant
1f7db01 
Signed-off-by: Michael Kinsner <michael.kinsner@intel.com>
@bader bader merged commit ee2b482 into intel:sycl Nov 14, 2019
@mkinsner mkinsner deleted the pipes_extension branch November 14, 2019 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@keryell keryell keryell left review comments

@asavonic asavonic Awaiting requested review from asavonic

@MrSidims MrSidims Awaiting requested review from MrSidims

Assignees

@bader bader

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mkinsner @keryell @bader @MrSidims