[SYCL] Implement initial host_pipe registration #5766

sherry-yuan · 2022-03-09T18:13:47Z

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.

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.

Spec link: #5838

CC: @pcolberg @aditikum @rho180

sherry-yuan · 2022-03-11T15:12:28Z

sycl/source/detail/host_pipe_map_entry.hpp

+struct HostPipeMapEntry {
+  // Pointer to the host_pipe on host.
+  void *MHostPipePtr;
+  // Size of the underlying type in the host_pipe.
+  std::uint32_t MHostPipeTSize;
+  // True if the host_pipe has been decorated with device_image_scope
+  bool MIsDeviceImageScopeDecorated;
+
+  // Constructor only initializes with the pointer to the host_pipe as the
+  // additional information is loaded after.
+  HostPipeMapEntry(void *HostPipePtr)
+      : MHostPipePtr(HostPipePtr), MHostPipeTSize(0),
+        MIsDeviceImageScopeDecorated(false) {}
+
+  void initialize(std::uint32_t HostPipeTSize,
+                  bool IsDeviceImageScopeDecorated) {
+    assert(HostPipeTSize != 0 && "Host pipe initialized with 0 size.");
+    assert(MHostPipeTSize == 0 && "Host pipe has already been initialized.");
+    MHostPipeTSize = HostPipeTSize;
+    MIsDeviceImageScopeDecorated = IsDeviceImageScopeDecorated;
+  }
+};


Main place of interest: Is there other information that needed to be encoded in each pipe_id to pipe_pointer mapping. Currently it only keeps track of the underlying data type's size and whether is device image scope decorated (which I doubt if it is needed), possibly need to see the final spec to determine what is needed.

sycl/source/detail/program_manager/program_manager.cpp

keryell

Quite interesting features.

keryell · 2022-03-12T00:45:45Z

sycl/include/CL/sycl/detail/pi.hpp

@@ -383,6 +383,13 @@ class DeviceBinaryImage {
    DeviceGlobals.init(Bin, __SYCL_PI_PROPERTY_SET_SYCL_DEVICE_GLOBALS);
    return DeviceGlobals;
  }
+  const PropertyRange getHostPipes() const {
+    // We can't have this variable as a class member, since it would break
+    // the ABI backwards compatibility.


This do-not-break-my-ABI story starts being a technical debt generator... :-(

Comment removed.

It was not against your comment, but more generally targeted at the project itself. @bader
@sherry-yuan So I want your great comment back! :-)

@keryell, I'm aware of that problem. I think we should start tracking these in terms of tasks for the future, when we can break old ABI compatibility.
@s-kanaev, do we track technical debt caused by ABI compatibility requirement?

@keryell, I'm aware of that problem. I think we should start tracking these in terms of tasks for the future, when we can break old ABI compatibility. @s-kanaev, do we track technical debt caused by ABI compatibility requirement?

I know @pvchupin has a link where it is tracked

Adding TODO here (like

llvm/sycl/include/CL/sycl/stream.hpp

Line 802 in 769851c

// TODO: This field is not used anymore.

) would be a good idea.
@s-kanaev, list you generated recently could be updated with this one.

Thanks! I will add //TODO in a separate PR that annotated all places in this files that will require changes after ABI policy change.

Updated the list.

The TODOs are annotated in #5929

sycl/source/detail/program_manager/program_manager.cpp

keryell · 2022-03-12T00:56:14Z

@Ralender @stsoe how does this map to host-pipes/host-streams in XRT? To investigate...

sherry-yuan · 2022-03-16T19:31:00Z

@GarveyJoe @rho180 @pcolberg

Let me know if there is any additional information that needs to be encoded/added in the class (for integration with frontend, and integration with runtime). The main place of interest is commented up there:

#5766 (comment)
#5766 (comment)

sherry-yuan · 2022-03-24T15:44:27Z

Hi all, the suggestions are applied accordingly, the review could resume now. Thanks!

AlexeySachkov

PropertySetIO changes LGTM.

It looks like the patch don't have a unit-test for the runtime changes. I honestly have no idea how to write those, but @intel/llvm-reviewers-runtime should be able to help here

sherry-yuan · 2022-03-24T16:11:17Z

Thanks for the review!

It looks like the patch don't have a unit-test for the runtime changes.

The test will only be meaningful if the frontend changes are also implemented. This change set up the required structure for storing the information of the host pipe (which compilation should cover what it is supposed to do). Once the frontend change & integration footer is done, then a unit test could be written to check if the whole flow works as expected.

(do let me know if there is a way to test this specific part tho)

sycl/source/detail/host_pipe_map_entry.hpp

sycl/source/detail/program_manager/program_manager.cpp

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 List of suported properties: intel#5839 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.

smaslov-intel

PI API changes LGTM

steffenlarsen

LGTM!

sherry-yuan · 2022-03-30T17:23:36Z

@intel/llvm-gatekeepers This change is ready to merge.

The test failures are known issues that are not related to this change, as it was also discovered in another PR.

Edit, the test re-run fails because of the known issue commented here: #5924 (comment)

sycl/source/detail/program_manager/program_manager.cpp

s-kanaev

The changes look good.
Though, are there any tests to be added for this functionality?
Unit-tests or E2E ones (in intel/llvm-test-suite)?

sherry-yuan · 2022-03-30T21:04:12Z

The changes look good.
Though, are there any tests to be added for this functionality?
Unit-tests or E2E ones (in intel/llvm-test-suite)?

Thanks @s-kanaev for the comments. This change is intended to unblock future development on integration footer (by exposing the host_pipe_map ::add function) and the registration of host pipes (i.e new changes might be added to these functions after future implementations). The test will not be meaningful until the integration footer change is in (it will be a change similar to this one: a9ad3af).
There will be likely an small integration test in test suite repo once the integration footer change is also implemented.

s-kanaev · 2022-03-31T18:43:02Z

There will be likely an small integration test in test suite repo once the integration footer change is also implemented.

Got it. Thank you.

Setup lower runtime extension functions for host pipes. See also intel#5766 intel#5851 Host pipe sycl spec: intel#5838

zibaiwan · 2022-11-21T13:49:56Z

Please make sure don't delete this. I am porting this to a new PR.

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.

Disclaimer: This work is a continuation of a previous approved Sherry's PR: #5766 and her draft work #5894. We are implementing Hostpipes based on the Spec [here](https://github.com/rho180/llvm/blob/rho180-pipe-design/sycl/doc/design/HostPipes.md). OpenCL spec is [here](https://github.com/intel-sandbox/ip-authoring-specs/blob/MJ_ChangeDocs4/Pipe/Spec/cl_intel_host_pipe_symbol.asciidoc). The following is the outline of the design. 1. The host pipe properties need to be added to the device image (probably similar to the previous change here a9ad3af) 2.The frontend calls the [registration of device image, maybe similar to this code](https://github.com/intel/llvm/blob/af858c74e4be95b2163ce6ba545ce588ff0aca4c/sycl/source/detail/program_manager/program_manager.cpp#L2014): which is where the host pipe information is available. This is where the mapping from host pipe name to host pipe pointer is extracted. 3. The frontend also calls `host_pipe_map::add` to register/initialize the host pipes. This is the new function. We are not sure about the ordering of registration of device image / registration of host pipe. for which ever one that comes later, it need to initialize the remaining attribute of host pipe (such as its properties). 4. The opencl runtime will need to get a cl_program object, which is typically not available until the first kernel launch. To get a program object early on, the host pipe name/pointer to device image mapping is cached during registration. And when the specific host pipe is needed, build the program and get its ocl runtime representation. This is done in the first couple commits. 5. Since a host pipe read/write need to depend on other write operation finishing before it (including the inter kernel write). This means the pipe needs to know the dependency of kernel execution. For this reason, the host pipe read and write ocl function cannot be called with no dep event. therefore, it is implemented with handler , which is aware of the event that it is supposed to wait upon. This is done in the "Register new command group .." commit. 6. Unit test - mock a fake device image. - register the fake device image - register fake pipe with some name you specified - fake the opencl functionality, this can be done with unittest::PiMock::redefine --------- Co-authored-by: Sherry Yuan <sherry.yuan@intel.com> Co-authored-by: Ho, Robert <robert.ho@intel.com>

sherry-yuan force-pushed the host_pipe branch 7 times, most recently from 5a7360f to 8b8574d Compare March 11, 2022 15:09

sherry-yuan commented Mar 11, 2022

View reviewed changes

sycl/source/detail/program_manager/program_manager.cpp Outdated Show resolved Hide resolved

keryell reviewed Mar 12, 2022

View reviewed changes

sherry-yuan force-pushed the host_pipe branch from 2ff9e89 to 6b51b97 Compare March 21, 2022 16:14

sherry-yuan marked this pull request as ready for review March 23, 2022 15:22

sherry-yuan requested review from a team as code owners March 23, 2022 15:22

sherry-yuan requested a review from smaslov-intel March 23, 2022 15:22

This comment was marked as resolved.

Sign in to view

sherry-yuan force-pushed the host_pipe branch 3 times, most recently from 6f63eaa to aa8906e Compare March 24, 2022 15:41

AlexeySachkov previously approved these changes Mar 24, 2022

View reviewed changes

smaslov-intel previously approved these changes Mar 24, 2022

View reviewed changes

steffenlarsen reviewed Mar 24, 2022

View reviewed changes

sycl/source/detail/host_pipe_map_entry.hpp Outdated Show resolved Hide resolved

sherry-yuan dismissed stale reviews from smaslov-intel and AlexeySachkov via 9e689d5 March 24, 2022 17:20

sherry-yuan force-pushed the host_pipe branch 2 times, most recently from 9e689d5 to 5fb2f3a Compare March 24, 2022 18:15

steffenlarsen reviewed Mar 28, 2022

View reviewed changes

sycl/source/detail/program_manager/program_manager.cpp Outdated Show resolved Hide resolved

sherry-yuan force-pushed the host_pipe branch from 172e790 to b3b06fc Compare March 28, 2022 16:59

smaslov-intel approved these changes Mar 28, 2022

View reviewed changes

steffenlarsen approved these changes Mar 28, 2022

View reviewed changes

AlexeySachkov approved these changes Mar 29, 2022

View reviewed changes

sherry-yuan mentioned this pull request Mar 30, 2022

Annotated refactorization after ABI constraint relaxed #5929

Closed

s-kanaev reviewed Mar 30, 2022

View reviewed changes

sycl/source/detail/program_manager/program_manager.cpp Show resolved Hide resolved

s-kanaev reviewed Mar 30, 2022

View reviewed changes

sherry-yuan added a commit to sherry-yuan/llvm that referenced this pull request Apr 21, 2022

Add pi extension API for host pipes

e4d513c

Setup lower runtime extension functions for host pipes. See also intel#5766 intel#5851 Host pipe sycl spec: intel#5838

github-actions bot added the Stale label Sep 28, 2022

github-actions bot closed this Oct 28, 2022

zibaiwan mentioned this pull request Dec 8, 2022

[SYCL] Host pipe runtime implementation #7468

Merged

[SYCL] Implement initial host_pipe registration #5766

[SYCL] Implement initial host_pipe registration #5766

Uh oh!

Conversation

sherry-yuan commented Mar 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sherry-yuan Mar 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

keryell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sherry-yuan Mar 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

keryell commented Mar 12, 2022

Uh oh!

sherry-yuan commented Mar 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as resolved.

sherry-yuan commented Mar 24, 2022

Uh oh!

AlexeySachkov left a comment

Choose a reason for hiding this comment

Uh oh!

sherry-yuan commented Mar 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

smaslov-intel left a comment

Choose a reason for hiding this comment

Uh oh!

steffenlarsen left a comment

Choose a reason for hiding this comment

Uh oh!

sherry-yuan commented Mar 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

s-kanaev left a comment

Choose a reason for hiding this comment

Uh oh!

sherry-yuan commented Mar 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

s-kanaev commented Mar 31, 2022

Uh oh!

zibaiwan commented Nov 21, 2022

Uh oh!

Uh oh!

sherry-yuan commented Mar 9, 2022 •

edited

Loading

sherry-yuan Mar 11, 2022 •

edited

Loading

sherry-yuan Mar 28, 2022 •

edited

Loading

sherry-yuan commented Mar 16, 2022 •

edited

Loading

sherry-yuan commented Mar 24, 2022 •

edited

Loading

sherry-yuan commented Mar 30, 2022 •

edited

Loading

sherry-yuan commented Mar 30, 2022 •

edited

Loading