Skip to content

[SYCL][DOC] Add Extension for FPGA task_sequence #6348

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 63 commits into from
Jul 8, 2024
Merged

[SYCL][DOC] Add Extension for FPGA task_sequence #6348

merged 63 commits into from
Jul 8, 2024

Conversation

rho180
Copy link
Contributor

@rho180 rho180 commented Jun 23, 2022 •
edited by tiwaria1
Loading

This PR:

  1. Adds a new Intel FPGA experimental SYCL extension that introduces support for task_sequences which provides a sub kernel task level parallelism interface.
  2. Updates the fpga_kernel_interface SYCL extension to add the stall_enable_clusters and stall_free_clusters properties. These are used with task sequences as well.

@rho180 rho180 requested a review from a team as a code owner June 23, 2022 14:57
rho180 and others added 4 commits July 4, 2022 11:11
@rho180 @steffenlarsen @GarveyJoe
Apply suggestions from code review
9eff282 
Co-authored-by: Steffen Larsen <[email protected]>
Co-authored-by: GarveyJoe <[email protected]>
@rho180
Update sycl_ext_intel_fpga_task_sequence.asciidoc
bc0b184 
Update response_capacity property definition. Remove 'single task' language from forward progress section.
@rho180
Update sycl_ext_intel_fpga_task_sequence.asciidoc
c99d4dd 
Update properties, fixes to task_sequence class def.
@rho180
Update sycl_ext_intel_fpga_task_sequence.asciidoc
9e3ded7
tiwaria1
tiwaria1 suggested changes Oct 5, 2022
View reviewed changes
rho180 and others added 3 commits December 5, 2022 10:36
@rho180
Merge branch 'intel:sycl' into sycl
3207da3
@tiwaria1
update format, limit line length
3ed1006
@tiwaria1
address minor review comments
61e9fc0 
- Add other property type traits
- Mention full name in overview, add note for short name
- change struct task_sequence to class task_sequence
- add note about pipelined property
keryell
keryell reviewed Jan 26, 2023
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.

Please start with a commented compelling code sample before diving into the details of the API.

@tiwaria1
Copy link
Contributor

tiwaria1 commented Jan 30, 2023 •
edited
Loading

Please start with a commented compelling code sample before diving into the details of the API.

Yup I have a unaddressed comment I filed on that. I will add some code samples in the next update :)

@tiwaria1
add example, addres review comments
3413ad5 
1. Fix link to kernel properties
2. Clarify effect of multiple async calls
3. Fix Backend Support Status section
4. methods -> member function
5. Correct section that talks about applying property to task_sequence
@tiwaria1
move examples to after overview section
601aa0e
@tiwaria1
correct kernel props link
a3df56d
@tiwaria1
Copy link
Contributor

tiwaria1 commented Feb 3, 2023

ping @steffenlarsen , please take another look. Thank you! :)

dm-vodopyanov pushed a commit that referenced this pull request Mar 12, 2024
@bowenxue-intel
[SYCL][FPGA] Implement task_sequence header, properties, and add new …
7b9001e 
…fpga_cluster kernel property (#12453)

Implement task_sequence header, task_sequence properties, and
fpga_cluster kernel property according to the spec updates
#6348 (almost ready to be merged)
Header
- Refactor invocation and response capacity to be on the Create
intrinsic, so Async and Get do not take in respective argument
- Remove max_outstanding argument of Create intrinsic
- Add pipelined and fpga_cluster arguments to Create intrinsic
- Async, Get, and Release intrinsic now accept the TargetExtType
target("spirv.TaskSequenceINTEL") returned from the Create intrinsic,
and do not take in object pointer or task function pointer arguments
- Task Sequence accepts properties

Task Sequence Properties
- Convert invocation and response capacity from template parameters to
properties
- Add balanced property to remove Get intrinsic loop in destructor

FPGA Kernel properties
- Add fpga_cluster property
- Support fpga_cluster and pipelined property for task sequence
@GarveyJoe GarveyJoe self-requested a review June 12, 2024 19:51
steffenlarsen
steffenlarsen reviewed Jun 21, 2024
View reviewed changes
Copy link
Contributor

@steffenlarsen steffenlarsen left a comment

Choose a reason for hiding this comment

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

I think it looks good, but I want to resolve the topics we discussed offline, specifically whether we should remove the feature test macro from the existing code and move it to proposed, until the open issues are resolved and we can consider it supported in intel/llvm.

@aejjehint
Copy link
Contributor

I think it looks good, but I want to resolve the topics we discussed offline, specifically whether we should remove the feature test macro from the existing code and move it to proposed, until the open issues are resolved and we can consider it supported in intel/llvm.

@steffenlarsen I have moved the extension to proposed. As for the issue regarding the test macro, the definition of the macro in intel/llvm will be removed once I roll back the change that introduced it a few months ago (#12453). As such, the definition of the macro as described in this spec will be viable as no product has shipped with the macro definition. Once the spec is finalized and checked in, and we are ready to re-apply the headers change I will put the macro definition back in and move the spec to experimental. Let me know if you have any other concerns.

steffenlarsen
steffenlarsen approved these changes Jun 25, 2024
View reviewed changes
Copy link
Contributor

@steffenlarsen steffenlarsen left a comment

Choose a reason for hiding this comment

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

Sounds good to me. 👍

@aejjehint aejjehint requested a review from GarveyJoe June 25, 2024 18:35
@steffenlarsen steffenlarsen merged commit 86efa23 into intel:sycl Jul 8, 2024
2 checks passed
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

@jessicadavies-intel jessicadavies-intel jessicadavies-intel left review comments

@bowenxue-intel bowenxue-intel bowenxue-intel left review comments

@aejjehint aejjehint aejjehint left review comments

@GarveyJoe GarveyJoe GarveyJoe approved these changes

@steffenlarsen steffenlarsen steffenlarsen approved these changes

@tiwaria1 tiwaria1 tiwaria1 approved these changes

@Pennycook Pennycook Awaiting requested review from Pennycook

@gmlueck gmlueck Awaiting requested review from gmlueck

@AerialMantis AerialMantis Awaiting requested review from AerialMantis

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

Successfully merging this pull request may close these issues.