Skip to content

[SYCL][FPGA] Rework blocking pipes #318

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 2 commits into from
Jul 18, 2019
Merged

[SYCL][FPGA] Rework blocking pipes #318

merged 2 commits into from
Jul 18, 2019

Conversation

MrSidims
Copy link
Contributor

@MrSidims MrSidims commented Jul 15, 2019 •
edited by bader
Loading

Replace workaround of using loop wrapper on a read/write pipe
function call with a call of blocking function.

@MrSidims MrSidims requested a review from AlexeySotkin July 15, 2019 14:49
agozillon
agozillon reviewed Jul 15, 2019
View reviewed changes
agozillon
agozillon reviewed Jul 15, 2019
View reviewed changes
keryell
keryell reviewed Jul 15, 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.

Good feature!
The non-blocking API seems very verbose but it can be simplified later or anyone can develop a C++ library anyway on top of it.
There are still a few comments...

sycl/doc/extensions/SYCL_pipes_usage.rst Outdated

Consider following code:
.. code:: cpp
template <class name, typename dataT, size_t min_capacity = 0> class pipe;
Copy link
Contributor

Choose a reason for hiding this comment

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

typename name?

@bader
Copy link
Contributor

bader commented Jul 15, 2019

The non-blocking API seems very verbose but it can be simplified later or anyone can develop a C++ library anyway on top of it.

@keryell, does it mean you are okay to merge non-blocking pipes i.e. #292?

@keryell
Copy link
Contributor

keryell commented Jul 15, 2019

The non-blocking API seems very verbose but it can be simplified later or anyone can develop a C++ library anyway on top of it.

@keryell, does it mean you are okay to merge non-blocking pipes i.e. #292?

I am lost now. What is the goal 2 different PR #318 and #292 ?

@keryell keryell mentioned this pull request Jul 15, 2019
Merged
@bader
Copy link
Contributor

bader commented Jul 15, 2019

The non-blocking API seems very verbose but it can be simplified later or anyone can develop a C++ library anyway on top of it.

@keryell, does it mean you are okay to merge non-blocking pipes i.e. #292?

I am lost now. What is the goal 2 different PR #318 and #292 ?

#292 adds common infrastructure for pipes + non-blocking operations.
#318 is built on top of #292, so @MrSidims added patches from #292 to #318.

I'd like to commit #292 first and rebase #318 after that.
Are you okay with this plan?

@MrSidims
Copy link
Contributor Author

The non-blocking API seems very verbose but it can be simplified later or anyone can develop a C++ library anyway on top of it.

@keryell, does it mean you are okay to merge non-blocking pipes i.e. #292?

I am lost now. What is the goal 2 different PR #318 and #292 ?

Sorry for the mess with PRs, I didn't mean to :(

This one is about adding blocking pipe instructing in SPIR-V translator and using SPIR-V friendly mangled functions in the header. So it depends on a #292.

@MrSidims MrSidims force-pushed the private/MrSidims/BlockingGlobalPipes_WOCL branch from 580f732 to adeee75 Compare July 15, 2019 20:54
asavonic
asavonic reviewed Jul 15, 2019
View reviewed changes
MrSidims added 2 commits July 17, 2019 13:54
@MrSidims
Add SPIR-V blocking pipe instructions
121de0b 
Signed-off-by: Dmitry Sidorov <dmitry.sidorov@intel.com>
@MrSidims
[SYCL][FPGA] Rework blocking pipes
cae4bc3 
Replace workaround of using loop wrapper on a read/write
pipe function call with a call of blocking functions.

Signed-off-by: Dmitry Sidorov <dmitry.sidorov@intel.com>
@MrSidims MrSidims force-pushed the private/MrSidims/BlockingGlobalPipes_WOCL branch from adeee75 to cae4bc3 Compare July 17, 2019 10:56
@bader
Copy link
Contributor

bader commented Jul 17, 2019

@keryell, @agozillon are you okay to merge this PR?

@agozillon
Copy link
Contributor

agozillon commented Jul 18, 2019 •
edited
Loading

Yeah LGTM, thanks for checking/asking.

keryell
keryell approved these changes Jul 18, 2019
View reviewed changes
Copy link
Contributor

@keryell keryell left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Nice to have blocking pipes!
It will reduce a little bit the global carbon footprint for applications running on FPGA... :-)

@bader bader merged commit 2b89aa0 into intel:sycl Jul 18, 2019
@keryell keryell mentioned this pull request Sep 17, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asavonic asavonic asavonic left review comments

@bader bader bader approved these changes

@keryell keryell keryell approved these changes

@agozillon agozillon agozillon approved these changes

@AlexeySotkin AlexeySotkin Awaiting requested review from AlexeySotkin

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@MrSidims @bader @keryell @agozillon @asavonic