-
Notifications
You must be signed in to change notification settings - Fork 801
[SYCL] [FPGA] Add latency control feature to FPGA extension docs #4980
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.
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
Conversation
sycl/doc/extensions/DataFlowPipes/data_flow_pipes_rev4_proposed.asciidoc
Outdated
Show resolved
Hide resolved
sycl/doc/extensions/DataFlowPipes/data_flow_pipes_rev4_proposed.asciidoc
Outdated
Show resolved
Hide resolved
According to intel#4980 this built-in should take four more constant integer parameters with following default values: ``` const int32_t AnchorID = -1 const int32_t TargetAnchor = 0 const int32_t Type = 0 const int32_t Cycle = 0 ``` The old variant is still allowed for backward compatibility. In case some parameters are not provided - default value will be emitted in annotation string.
676de1f to
4c7ed85
Compare
gmlueck
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed a couple of typos below.
We also had a meeting on Friday to resolve some final issues with the compile-time property API, and this will result in some small name changes to your properties. @rolandschulz has the AR to update the compile-time property extension specification, which will document these changes. It might be easier to wait for that before merging this PR, so that you don't need to update it again later.
I think the changes are:
-
The struct that defines the property should have the suffix "_key" (e.g.
latency_constraint_key). -
Enums like
latency_constraint::typeshould be declared outside of the property struct and prefixed with the name of the property. For example:enum class latency_constraint_type { none, exact, max, min }; -
There should be a namespace scope variable without the "_key" suffix that applications can use to create the
propertiesobject. For example:template<int Target, type Type, int Cycle> constexpr inline latency_constraint_key::value_t<Target, Type, Cycle> latency_constraint;Actually, the preferred style is normally to create one such variable for each enumerator:
template<int Target, int Cycle> constexpr inline latency_constraint_key::value_t<Target, latency_constraint_type::none, Cycle> latency_constraint_none; template<int Target, int Cycle> constexpr inline latency_constraint_key::value_t<Target, latency_constraint_type::exact, Cycle> latency_constraint_exact; // Etc.I'm trying to decide if this is a good style in your case, though, since this property has two other integer parameters.
sycl/doc/extensions/DataFlowPipes/data_flow_pipes_rev4_proposed.asciidoc
Outdated
Show resolved
Hide resolved
|
@gmlueck In that case I'll wait for finalization of property API now. |
4c7ed85 to
5b788be
Compare
I think I am waiting for @shuoniu-intel to update with that latest property API:
|
|
Close this PR because we will deliver experimental version API for latency control in oneAPI 2022.2 release. |
This is a continuation of @Fznamznon's PR #5002. Most of the changes here are from there. According to #4980 this built-in should take four more constant integer parameters with following default values: const int32_t AnchorID = -1 const int32_t TargetAnchor = 0 const int32_t Type = 0 const int32_t Cycle = 0 The old variant is still allowed for backward compatibility. In case some parameters are not provided - default value will be emitted in annotation string. In addition to the state of PR #5002, this PR makes minor changes. It incorporates @mlychkov's code review comments there and modifies the CodeGenSYCL test to accept both opaque and non-opaque pointers.
Latency control parameters will be passed to FPGA pipe and FPGA LSU member functions via
Properties. Note that the name convention ofPropertiestype is not finalized yet, and usage ofPropertiesin these two extension documents can be revised once the decisions firm up.One minor change in FPGALsu.md: removing the
cl::namespace according to the SYCL 2020 spec.This PR is ported from #4917.