Skip to content

[SYCL] [FPGA] Update the experimental latency control API to use property list #5993

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 10 commits into from
Apr 28, 2022
Merged

[SYCL] [FPGA] Update the experimental latency control API to use property list #5993

merged 10 commits into from
Apr 28, 2022

Conversation

shuoniu-intel
Copy link
Contributor

@shuoniu-intel shuoniu-intel commented Apr 8, 2022 •
edited
Loading

As planned before, this patch deprecates the template argument approach in the experimental latency control API, and use property list instead.
I'm still updating the experimental API rather than the formal API because the frontend support (clang and SPIR-V) for latency control is not ready yet, so I'd like to wait for them before fully moving on to deprecate the experimental API.

This patch contains three parts:

  • Update the extension documents for pipes and LSUs, change the experimental latency control to use property list in the API.
    • sycl/doc/extensions/supported/sycl_ext_intel_dataflow_pipes.asciidoc
    • sycl/doc/extensions/supported/sycl_ext_intel_fpga_lsu.md
  • Create properties for latency control.
    • sycl/include/sycl/ext/oneapi/latency_control/properties.hpp
    • sycl/include/sycl/ext/oneapi/properties/property.hpp
    • sycl/test/extensions/properties/properties_latency_control.cpp
  • Update headers for the experimental latency control, change the API to use property list.
    • sycl/include/sycl/ext/intel/experimental/fpga_utils.hpp
    • sycl/include/sycl/ext/intel/experimental/fpga_lsu.hpp
    • sycl/include/sycl/ext/intel/experimental/pipes.hpp

Test: intel/llvm-test-suite#982

@shuoniu-intel shuoniu-intel force-pushed the latency-control-extension-doc branch from 1b97869 to 71281bc Compare April 8, 2022 19:49
@shuoniu-intel shuoniu-intel mentioned this pull request Apr 8, 2022
Merged
@shuoniu-intel shuoniu-intel force-pushed the latency-control-extension-doc branch 2 times, most recently from 4026698 to 70f863c Compare April 8, 2022 20:52
@shuoniu-intel shuoniu-intel force-pushed the latency-control-extension-doc branch from 70f863c to b299e0f Compare April 8, 2022 21:35
@shuoniu-intel shuoniu-intel marked this pull request as ready for review April 9, 2022 14:22
@shuoniu-intel shuoniu-intel requested review from a team as code owners April 9, 2022 14:22
@shuoniu-intel
Copy link
Contributor Author

Add @steffenlarsen to review the properties part.

keryell
keryell reviewed Apr 12, 2022
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.

Did I misread or this is mostly uncommented code? :-/

@shuoniu-intel shuoniu-intel requested a review from gmlueck April 12, 2022 14:06
steffenlarsen
steffenlarsen reviewed Apr 14, 2022
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.

The properties and the use thereof looks good to me, but I have some minor comments.

I also agree with @keryell that some additional comments here and there wouldn't hurt.

@shuoniu-intel shuoniu-intel force-pushed the latency-control-extension-doc branch from 8cb91af to f4968f8 Compare April 18, 2022 15:45
@shuoniu-intel shuoniu-intel force-pushed the latency-control-extension-doc branch from 62bdae8 to ec1449b Compare April 19, 2022 00:45
@keryell
Copy link
Contributor

keryell commented Apr 19, 2022

I guess the naming with leading underscores is common in C/C++ standard headers to avoid conflict with user-defined macros, since it is forbidden in C/C++ to have user-defined identifiers starting with _ and uppercase letter or __.
The problem here is that was 1 _ followed by lowercase letters, which can conflict...

@shuoniu-intel
Copy link
Contributor Author

We need another separate patch to clean up naming in FPGA headers.
To Change _snake_case to CamelCase.

@shuoniu-intel shuoniu-intel requested a review from gmlueck April 19, 2022 15:02
steffenlarsen
steffenlarsen reviewed Apr 21, 2022
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.

Only one more minor comment, then I think I am happy with it.

steffenlarsen
steffenlarsen approved these changes Apr 25, 2022
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.

LGTM!

gmlueck
gmlueck approved these changes Apr 25, 2022
View reviewed changes
Copy link
Contributor

@gmlueck gmlueck left a comment

Choose a reason for hiding this comment

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

Spec changes LGMT.

@steffenlarsen
Copy link
Contributor

Failures either unrelated or fixed in intel/llvm-test-suite#982.

@steffenlarsen steffenlarsen merged commit 273034a into intel:sycl Apr 28, 2022
steffenlarsen pushed a commit to intel/llvm-test-suite that referenced this pull request Apr 28, 2022
@shuoniu-intel
[SYCL] [FPGA] Update latency control E2E tests (#982)
2b77270 
Update latency control E2E tests to use property list in the API.

The corresponding SYCL PR: intel/llvm#5993
@shuoniu-intel shuoniu-intel mentioned this pull request Jun 13, 2022
Closed
4 tasks
aelovikov-intel pushed a commit to aelovikov-intel/llvm that referenced this pull request Mar 27, 2023
@shuoniu-intel
[SYCL] [FPGA] Update latency control E2E tests (intel/llvm-test-suite…
6c8a21a 
…#982)

Update latency control E2E tests to use property list in the API.

The corresponding SYCL PR: intel#5993
@shuoniu-intel shuoniu-intel deleted the latency-control-extension-doc branch October 19, 2023 17:17
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

@pvchupin pvchupin pvchupin left review comments

@gmlueck gmlueck gmlueck approved these changes

@steffenlarsen steffenlarsen steffenlarsen approved these changes

@againull againull Awaiting requested review from againull againull is a code owner automatically assigned from intel/llvm-reviewers-runtime

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
@shuoniu-intel @keryell @steffenlarsen @gmlueck @pvchupin