llvm icon indicating copy to clipboard operation
llvm copied to clipboard

[SYCL][DOC] Add Extension for FPGA task_sequence

Open rho180 opened this issue 3 years ago • 9 comments

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 avatar Jun 23 '22 14:06 rho180

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 avatar Jan 30 '23 16:01 tiwaria1

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

tiwaria1 avatar Feb 03 '23 13:02 tiwaria1

@intel/dpcpp-specification-reviewers please review this spec. Thank you! :)

tiwaria1 avatar Mar 09 '23 16:03 tiwaria1

Hello, @intel/dpcpp-specification-reviewers apologies for the repeated pings. Please review this extension. Thank you! :)

tiwaria1 avatar Mar 13 '23 14:03 tiwaria1

ping @intel/dpcpp-specification-reviewers for review, thank you :)

tiwaria1 avatar Mar 16 '23 17:03 tiwaria1

I think it would make sense to have a new sycl::aspect value for querying if a device supports task_sequence. Likewise we could have the runtime throw an exception if a kernel using task_sequence going to be used on a device that does not support it.

steffenlarsen avatar Mar 20 '23 12:03 steffenlarsen

I think it would make sense to have a new sycl::aspect value for querying if a device supports task_sequence. Likewise we could have the runtime throw an exception if a kernel using task_sequence going to be used on a device that does not support it.

I copied over the aspect description from the template. Is that sufficient or do you want to add anything else?

tiwaria1 avatar Mar 22 '23 14:03 tiwaria1

@intel/dpcpp-specification-reviewers Based on prior comments, it seems like most of the discussion points have been addressed and most of you have already rubber stamped it prior to the recent discussions and corresponding changes, so I believe this spec can be approved and merged.

bowenxue-intel avatar Jan 22 '24 20:01 bowenxue-intel

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 (https://github.com/intel/llvm/pull/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.

aejjehint avatar Jun 24 '24 19:06 aejjehint