Skip to content

[SYCL][Doc] Update SYCL_INTEL_data_flow_pipes extension for FPGA host pipe support #5838

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

Closed
wants to merge 3 commits into from

Conversation

pcolberg
Copy link
Contributor

@pcolberg pcolberg commented Mar 19, 2022

Add a memory order parameter to device-side read/write members and
default to sycl::memory_order::seq_cst.

Replace min_capacity property with compile-time properties list for
use with SYCL_INTEL_FPGA_data_flow_pipes_properties extension.

Add host pipe read/write members with additional sycl::queue parameter.

Cc: @mkinsner @GarveyJoe @aditikum @rho180 @zibaiwan

… pipe support

Add a memory order parameter to device-side read/write members and
default to `sycl::memory_order::seq_cst`.

Replace `min_capacity` property with compile-time properties list for
use with `SYCL_INTEL_FPGA_data_flow_pipes_properties` extension.

Add host pipe read/write members with additional `sycl::queue` parameter.

Signed-off-by: Peter Colberg <peter.colberg@intel.com>
@pcolberg pcolberg requested a review from a team as a code owner March 19, 2022 00:45
@pcolberg pcolberg marked this pull request as draft March 19, 2022 00:47
@pcolberg pcolberg force-pushed the sycl_ext_intel_dataflow_pipes branch 3 times, most recently from 51c980c to fe48ecf Compare March 19, 2022 01:30
@pcolberg pcolberg force-pushed the sycl_ext_intel_dataflow_pipes branch 2 times, most recently from 1b78509 to ca64371 Compare March 19, 2022 16:56
@@ -116,7 +130,7 @@ A pipe type is a specialization of the pipe class:
----
template <typename name,
typename dataT,
size_t min_capacity = 0>
typename propertiesT = properties<>>
Copy link
Contributor

Choose a reason for hiding this comment

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

Using properties here is nice, but this is an API-breaking change to a supported extension. Existing code using the old API will no longer compiler, right?

Copy link
Contributor

@sherry-yuan sherry-yuan Mar 21, 2022

Choose a reason for hiding this comment

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

Its not possible to keep both class variant at the same time in experimental.

// existing implementation
template <class _name, class _dataT, int32_t _min_capacity = 0> 
class pipe {

};
// updated implementation
template <class _name, class _dataT, typename PropertyList>
class pipe {

};

Will give error:

./Playground/file0.cpp:4:62: error: template parameter 'int _min_capacity'
    4 | template <class _name, class _dataT, int32_t _min_capacity = 0>
      |                                                              ^
./Playground/file0.cpp:10:7: note: redeclared here as 'class PropertyList'
   10 | class pipe {
      |       ^~~~

The choice is between: breaking ABI vs defining a new name/namespace for pipes

Copy link
Contributor

Choose a reason for hiding this comment

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

This is exactly the difference between an "experimental" extension vs. a "supported" one. If it is "supported", we promise not to break API compatibility with existing code without going through a deprecation process.

If the FPGA team wants the freedom to change APIs without deprecation, we should consider making this an experimental API (which includes moving it to the the experimental namespace).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed this breaks the ABI, and I don't see any trick using SFINAE via std::enable_if to retain compatibility.

@GarveyJoe, am I missing anything obvious?

@gmlueck, would you accept a global macro to opt pipe into the properties parameter, along with a deprecation notice for the _min_capacity parameter when the macro is undefined? The properties parameter would become the default at some later point, after compile-time constant properties have been moved out of experimental.

Copy link
Contributor Author

@pcolberg pcolberg Mar 21, 2022

Choose a reason for hiding this comment

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

This is exactly the difference between an "experimental" extension vs. a "supported" one. If it is "supported", we promise not to break API compatibility with existing code without going through a deprecation process.

If the FPGA team wants the freedom to change APIs without deprecation, we should consider making this an experimental API (which includes moving it to the the experimental namespace).

Yes, this feature would be implemented in sycl::ext::intel::experimental::pipe for now, but that still leaves the question of how to allow for a deprecation period in which both the old and the new template parameter are supported once this is moved out of experimental.

Copy link
Contributor

Choose a reason for hiding this comment

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

@pcolberg @GarveyJoe @gmlueck Is there anything we still need to resolve in this conversation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The conclusion was to retain _min_capacity as the third parameter and add compile-time properties as a fourth parameter, which has been merged in #5886 for the experimental namespace.

sherry-yuan added a commit to sherry-yuan/llvm that referenced this pull request Mar 21, 2022
To align with spec: intel#5838

Host pipe is a FIFO construct that provide links between elements of design that are accessed through read/write API. Host pipe is a pipe that links device kernel with host program.
sherry-yuan added a commit to sherry-yuan/llvm that referenced this pull request Mar 25, 2022
Defines new properties for data flow pipes

Properties defined to align with spec in [1]

Existing data flow pipe defined in [2]

data flow pipe will be accepting new property list, spec in [3]

[1] intel#5839
[2] https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/proposed/sycl_ext_intel_dataflow_pipes.asciidoc
[3] intel#5838
sherry-yuan added a commit to sherry-yuan/llvm that referenced this pull request Mar 25, 2022
Setup lower runtime extension functions for host pipes.

See also
intel#5766
intel#5851

Host pipe sycl spec: intel#5838
@github-actions github-actions bot added the Stale label Feb 15, 2023
rho180 pushed a commit to zibaiwan/llvm that referenced this pull request Feb 24, 2023
What is this for: pipes expose the concept of a first in first out buffer,
 this FIFO construct provide link between elements of a design that are
accessed through read/write/push/pop APIs.
A host pipe is a pipe that links a device kernel with host program.
This extension is framed from FPGA perspective.

This change add required interface for the integration footer to register
the `host_pipe` of a program as well as reading extended info supplied
through "SYCL/host pipes" property.
Info is stored in a map managed by program manager.

The integration header and footer provides a mapping from the host address of
each pipe variable to the unique string for that variable.
This is required so that sycl runtime can query the pipe address from the
given pipe name, and pass both into opencl runtime function calls.

Opencl defines pipes, which are FIFO constructs that are consistent with Khronos specification.

Spec link: intel#5838

Note: it is the first change to runtime relating to host_pipe,
thus the feature is not complete / fully testable.
It is intended to add an interface for integration footer as well as
consumer for the information sycl-post-link will be generating when
future work is added.

(cherry picked from commit 031f829)

Zibai fixed all conflicts.
rho180 pushed a commit to zibaiwan/llvm that referenced this pull request Feb 24, 2023
Setup lower runtime extension functions for host pipes.

See also
intel#5766
intel#5851

Host pipe sycl spec: intel#5838

(cherry picked from commit e4d513c)

Zibai fixed the conflict.
rho180 pushed a commit to zibaiwan/llvm that referenced this pull request Feb 24, 2023
Defines the flow of enqueue new host pipe operations (read/write),
User provide the queue to enqueue this event, and the runtime queries
the pipe address from registration using the given address and unique ID.
The runtime pass the pipe name, and host address into queue submit of new command group.
The enqueued command calls new opencl function, and provide the current program, queue,
event wait list, pipe name, host pointer of the data destination.

Spec: intel#5838
(cherry picked from commit 12e9e85)

Zibai fixed all the conflicts.
zibaiwan pushed a commit to zibaiwan/llvm that referenced this pull request Mar 14, 2023
What is this for: pipes expose the concept of a first in first out buffer,
 this FIFO construct provide link between elements of a design that are
accessed through read/write/push/pop APIs.
A host pipe is a pipe that links a device kernel with host program.
This extension is framed from FPGA perspective.

This change add required interface for the integration footer to register
the `host_pipe` of a program as well as reading extended info supplied
through "SYCL/host pipes" property.
Info is stored in a map managed by program manager.

The integration header and footer provides a mapping from the host address of
each pipe variable to the unique string for that variable.
This is required so that sycl runtime can query the pipe address from the
given pipe name, and pass both into opencl runtime function calls.

Opencl defines pipes, which are FIFO constructs that are consistent with Khronos specification.

Spec link: intel#5838

Note: it is the first change to runtime relating to host_pipe,
thus the feature is not complete / fully testable.
It is intended to add an interface for integration footer as well as
consumer for the information sycl-post-link will be generating when
future work is added.

(cherry picked from commit 031f829)

Zibai fixed all conflicts.
zibaiwan pushed a commit to zibaiwan/llvm that referenced this pull request Mar 14, 2023
Setup lower runtime extension functions for host pipes.

See also
intel#5766
intel#5851

Host pipe sycl spec: intel#5838

(cherry picked from commit e4d513c)

Zibai fixed the conflict.
zibaiwan pushed a commit to zibaiwan/llvm that referenced this pull request Mar 14, 2023
Defines the flow of enqueue new host pipe operations (read/write),
User provide the queue to enqueue this event, and the runtime queries
the pipe address from registration using the given address and unique ID.
The runtime pass the pipe name, and host address into queue submit of new command group.
The enqueued command calls new opencl function, and provide the current program, queue,
event wait list, pipe name, host pointer of the data destination.

Spec: intel#5838
(cherry picked from commit 12e9e85)

Zibai fixed all the conflicts.
@github-actions
Copy link
Contributor

This pull request was closed because it has been stalled for 30 days with no activity.

@github-actions github-actions bot closed this Mar 18, 2023
steffenlarsen added a commit that referenced this pull request Mar 30, 2023
#8789)

A continuation of #5838. Accompanying runtime change is #7468.

Add a memory order parameter to device-side read/write members and
default to sycl::memory_order::seq_cst. This parameter is in place but
not being used at this moment, it's intended for the future work.

Add host pipe read/write members with additional sycl::queue parameter.

---------

Co-authored-by: Steffen Larsen <steffen.larsen@intel.com>
@pcolberg pcolberg deleted the sycl_ext_intel_dataflow_pipes branch March 15, 2025 03:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants