Skip to content

[SYCL][FPGA] Rework blocking pipes - #318

Merged
bader merged 2 commits into
intel:syclfrom
MrSidims:private/MrSidims/BlockingGlobalPipes_WOCL
Jul 18, 2019
Merged

[SYCL][FPGA] Rework blocking pipes#318
bader merged 2 commits into
intel:syclfrom
MrSidims:private/MrSidims/BlockingGlobalPipes_WOCL

Conversation

@MrSidims

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

Copy link
Copy Markdown
Contributor

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
Comment thread llvm-spirv/lib/SPIRV/OCLUtil.cpp Outdated
agozillon
agozillon reviewed Jul 15, 2019
View reviewed changes
Comment thread llvm-spirv/lib/SPIRV/OCLUtil.cpp Outdated
keryell
keryell reviewed Jul 15, 2019
View reviewed changes

@keryell keryell left a comment

Copy link
Copy Markdown
Contributor

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...

Comment thread llvm-spirv/lib/SPIRV/OCLUtil.cpp Outdated
Comment thread llvm-spirv/lib/SPIRV/OCLUtil.cpp Outdated
Comment thread sycl/doc/extensions/SYCL_pipes_usage.rst Outdated

@keryell keryell Jul 15, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

typename name?

Comment thread sycl/doc/extensions/SYCL_pipes_usage.rst Outdated
Comment thread sycl/doc/extensions/SYCL_pipes_usage.rst Outdated
Comment thread sycl/test/fpga_tests/fpga_pipes.cpp Outdated
Comment thread sycl/test/fpga_tests/fpga_pipes.cpp Outdated
Comment thread sycl/test/fpga_tests/fpga_pipes.cpp Outdated
Comment thread sycl/test/fpga_tests/fpga_pipes.cpp Outdated
Comment thread sycl/test/fpga_tests/fpga_pipes.cpp Outdated
@bader

bader commented Jul 15, 2019

Copy link
Copy Markdown
Contributor

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

keryell commented Jul 15, 2019

Copy link
Copy Markdown
Contributor

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

bader commented Jul 15, 2019

Copy link
Copy Markdown
Contributor

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

MrSidims commented Jul 15, 2019

Copy link
Copy Markdown
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
Comment thread llvm-spirv/lib/SPIRV/OCLUtil.cpp Outdated
Dmitry Sidorov added 2 commits July 17, 2019 13:54
Dmitry Sidorov
Add SPIR-V blocking pipe instructions
121de0b 
Signed-off-by: Dmitry Sidorov <[email protected]>
Dmitry Sidorov
[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 <[email protected]>
@MrSidims
MrSidims force-pushed the private/MrSidims/BlockingGlobalPipes_WOCL branch from adeee75 to cae4bc3 Compare July 17, 2019 10:56
@bader

bader commented Jul 17, 2019

Copy link
Copy Markdown
Contributor

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

@agozillon

agozillon commented Jul 18, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

Yeah LGTM, thanks for checking/asking.

keryell
keryell approved these changes Jul 18, 2019
View reviewed changes

@keryell keryell left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

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
adamfidel pushed a commit to adamfidel/llvm that referenced this pull request Feb 9, 2026
Maxime France-Pillois
[SYCL][Graph] Add support for enabling CommandBuffer submission profi…
253420a 
…ling (intel#318)

* [SYCL][Graph] Add e2e test checking profiling info

Adds an e2e test that checks the profiling info from an event returned by a graph submission.

Closes Issue: intel#96

* [SYCL][Graph] Update the implementation to support Profiling on graph execution

Sycl event impl object contains a flag indicating that this event results from a graph submission.
This flag was not set for every type of graph sumbission. This commit fixes this bug.
Adds a extra event in the first command list associated to the CommandBuffer execution to obtain the start time of the graph execution.
Modifies the urEventGetProfilingInfo function to get the CommandBuffer start time from this new event.
Improves to profiling e2e test.
Removes the test checking for exception throwing (unsupported feature).

* [SYCL][Graph] Add resync of submit time to start time if time shift

* [SYCL][Graph] Adds condition on queue prop before adding the new profiling event to CL
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bader bader bader approved these changes
@AlexeySotkin AlexeySotkin Awaiting requested review from AlexeySotkin
+3 more reviewers
@asavonic asavonic asavonic left review comments
@keryell keryell keryell approved these changes
@agozillon agozillon agozillon approved these changes
Reviewers whose approvals may not affect merge requirements

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