[SYCL] Allow fpga_reg only for PODs and Trivially-copyable structs #3643

ajaykumarkannan · 2021-04-28T15:51:54Z

This change restricts the INTEL::fpga_reg function to only take in trivially copyable structs, and models the hardware better but created a registered copy, rather than just passing through as a reference.

The intention of this function was initially built for PODs and later extended to structs in OpenCL. It literally is used to force a register stage in hardware between the input and output (i.e. in the software model, it is an exact copy of the data in memory). It doesn't translate directly to C++ objects because classes with complex copy constructors cannot be modelled to do what is described above, and building out this builtin in hardware as per the software model reduces its use case. As such, restricting the function to only types that have a usable and correct implementation of this function is the ideal thing to do here

Note that I also removed the old mapping to intelfpga::fpga_reg as part of this changelist.

Testing:

Ran this new header file with a variety of SYCL examples in the FPGA design suite to ensure that it was still behaving in its intended behavior

and imply that a copy is created so that the x86 and the FPGA model are equivalent. We want to only allow trivially copyable structs, because we're creating an exact copy on device.

bader · 2021-04-29T07:39:51Z

Note that I also removed the old mapping to intelfpga::fpga_reg as part of this changelist.

I think the process for removing APIs includes adding a deprecation warning about removing an API in future releases and some time period for our users to transition to a new API.
Do you think this change won't impact any users?

mkinsner · 2021-04-29T12:14:59Z

Do we definitely want is_trivially_copyable instead of sycl::is_device_copyable?

jessicadavies-intel · 2021-04-29T12:30:25Z

What is the error message if fpga_reg is passed something that is not trivially copyabe?

ajaykumarkannan · 2021-04-29T15:53:46Z

Note that I also removed the old mapping to intelfpga::fpga_reg as part of this changelist.

I think the process for removing APIs includes adding a deprecation warning about removing an API in future releases and some time period for our users to transition to a new API.
Do you think this change won't impact any users?

@bader I just reviewed our gold docs and we used the INTEL::fpga_reg syntax. I believe the intelfpga::fpga_reg only existed temporarily during the beta phase, so I don't believe we'll need a deprecation warning here.

ajaykumarkannan · 2021-04-29T16:01:11Z

Also, do I need to include <type_traits> here since I'm using std::enable_if? It didn't seem to be necessary on dpcpp with my example designs, but that may have been due to other dependencies?

Yes <type_traits> needs to be included.

ajaykumarkannan · 2021-04-29T16:04:45Z

What is the error message if fpga_reg is passed something that is not trivially copyabe?

@jessicadavies-intel

https://godbolt.org/z/ca41TKzMv

Clang Error:

<source>:32:14: error: no matching function for call to 'fpga_reg'
    auto t = sycl::INTEL::fpga_reg(X);
             ^~~~~~~~~~~~~~~~~~~~~
<source>:13:1: note: candidate template ignored: requirement 'std::is_trivially_copyable<my_obj>::value' was not satisfied [with _T = my_obj]
fpga_reg(_T t) {
^
1 error generated.
Compiler returned: 1

GCC Error:

<source>: In function 'int main()':
<source>:32:35: error: no matching function for call to 'fpga_reg(my_obj&)'
   32 |     auto t = sycl::INTEL::fpga_reg(X);
      |              ~~~~~~~~~~~~~~~~~~~~~^~~
<source>:13:1: note: candidate: 'template<class _T> typename std::enable_if<std::is_trivially_copyable<_Tp>::value, _T>::type sycl::INTEL::fpga_reg(_T)'
   13 | fpga_reg(_T t) {
      | ^~~~~~~~
<source>:13:1: note:   template argument deduction/substitution failed:
<source>: In substitution of 'template<class _T> typename std::enable_if<std::is_trivially_copyable<_Tp>::value, _T>::type sycl::INTEL::fpga_reg(_T) [with _T = my_obj]':
<source>:32:35:   required from here
<source>:13:1: error: no type named 'type' in 'struct std::enable_if<false, my_obj>'
Compiler returned: 1

ajaykumarkannan · 2021-04-29T16:10:01Z

Do we definitely want is_trivially_copyable instead of sycl::is_device_copyable?

@mkinsner I think yes. sycl::is_device_copyable is a superset and I think we want to cover those types as well. @mkinsner do you know if the implementations of the types listed under sycl::is_device_copyable (e.g. std::array, std::tuple, std::pair) fit within the description I mentioned? I think we would need a bit more testing to ensure that they are compatible here though.

bader · 2021-05-11T17:00:28Z

@jessicadavies-intel, @mkinsner, @pvchupin, do you have any objections to merge this?

pvchupin · 2021-05-11T17:50:53Z

I don't have objections but it would be good if FPGA folks can approve this, having backward compatibility concerns.

sycl/include/CL/sycl/INTEL/fpga_reg.hpp

vladimirlaz

Approve to start tests

mkinsner · 2021-05-13T11:52:45Z

Do we definitely want is_trivially_copyable instead of sycl::is_device_copyable?

@mkinsner I think yes. sycl::is_device_copyable is a superset and I think we want to cover those types as well. @mkinsner do you know if the implementations of the types listed under sycl::is_device_copyable (e.g. std::array, std::tuple, std::pair) fit within the description I mentioned? I think we would need a bit more testing to ensure that they are compatible here though.

We can test and relax in a future change, if desired. tuple, for example, will be made illegal by the trivially copyable restriction, and I thing we'd want to allow such types long term.

…tion signature

keryell · 2021-05-18T05:09:44Z

sycl/include/CL/sycl/INTEL/fpga_reg.hpp

-fpga_reg(_T t) {
+template <typename _T> _T fpga_reg(_T t) {
+  static_assert(std::is_trivially_copyable<_T>::value,
+                "Type is not trivially_copyable.");


Yes, that looks simple and understandable. :-)

ajaykumarkannan · 2021-05-25T14:57:38Z

@keryell had to change it to do deprecation warnings instead for at least 2021.4. Can you please re-review this from the usability point of view? Thanks!

kbsmith-intel

LGTM

sycl/include/CL/sycl/INTEL/fpga_reg.hpp

ajaykumarkannan · 2021-06-08T15:04:17Z

@mkinsner @MrSidims bump. We converted to a deprecation warning instead. Could you please re-review? Thanks

* upstream/sycl: (649 commits) [SYCL][Driver][NFC] Update integration footer test for 32-bit host (intel#4039) [SYCL][L0] Initialize descriptor .stype and .pNext (intel#4032) [SYCL] Add sycl::kernel::get_kernel_bundle method (intel#3855) [SYCL] Add support for device UUID as a SYCL extension. (intel#3696) [SYCL][Matrix] Add spec document for the matrix extension interface and its first implementation for AMX (intel#3551) Fix debug build mangler test after PR#3992 (8f38045). (intel#4033) [Driver][SYCL] Restrict user -include file in final integration footer step (intel#4036) [SYCL] [Tests] Do not copy device binary image mocks (intel#4023) [SYCL][Doc] Update docs to reflect new compiler features (intel#4030) [SYCL][CUDA] cl_khr_fp16 extension connected to cuda PI. (intel#4029) [SYCL][NFC] Refactor RT unit tests (intel#4021) [SYCL] Switch to using integration footer by default (intel#3777) [SYCL][CUDA] Add the Use Default Stream property (intel#4004) Uplift GPU RT version for Linux to 21.24.20098 (intel#4003) [SYCL][CUDA] atomic_ref.fetch_add used for fp64 reduction if device.has(atomic64) (intel#3950) [Driver][SYCL] Differentiate host dependency link from regular host link (intel#4002) [SYCL][ESIMD] Support device half type in intrinsics. (intel#4024) [SYCL] Allow fpga_reg only for PODs and Trivially-copyable structs (intel#3643) [SYCL][FPGA] Restore legacy debug info version for the hardware (intel#3991) [SYCL][PI][L0] Force reset of memcpy command-list. (intel#4001) ...

ajaykumarkannan added 3 commits April 13, 2021 16:07

Change fpga_reg interface to only allow trivially copyable structs,

068788c

and imply that a copy is created so that the x86 and the FPGA model are equivalent. We want to only allow trivially copyable structs, because we're creating an exact copy on device.

Add newline at EOF

18696ab

Fix a typo in fpga_reg.hpp

1fa673b

ajaykumarkannan requested a review from a team as a code owner April 28, 2021 15:51

ajaykumarkannan requested a review from vladimirlaz April 28, 2021 15:51

MrSidims previously approved these changes Apr 28, 2021

View reviewed changes

vladimirlaz previously approved these changes Apr 29, 2021

View reviewed changes

bader changed the title ~~Allow fpga_reg only for PODs and Trivially-copyable structs~~ [SYCL] Allow fpga_reg only for PODs and Trivially-copyable structs Apr 29, 2021

bader requested review from pvchupin, jessicadavies-intel and mkinsner May 11, 2021 17:00

pvchupin requested a review from kbsmith-intel May 11, 2021 17:54

kbsmith-intel reviewed May 11, 2021

View reviewed changes

sycl/include/CL/sycl/INTEL/fpga_reg.hpp Show resolved Hide resolved

sycl/include/CL/sycl/INTEL/fpga_reg.hpp Show resolved Hide resolved

Add type_traits as include to fpga_reg.hpp

37739ac

ajaykumarkannan dismissed stale reviews from vladimirlaz and MrSidims via 37739ac May 13, 2021 04:35

vladimirlaz requested review from kbsmith-intel, vladimirlaz and MrSidims May 13, 2021 05:27

vladimirlaz previously approved these changes May 13, 2021

View reviewed changes

vladimirlaz self-requested a review May 13, 2021 05:28

mkinsner previously approved these changes May 13, 2021

View reviewed changes

Using a static assert in fpga_reg instead of a type_trait in the func…

ccca3ba

…tion signature

ajaykumarkannan dismissed stale reviews from mkinsner and vladimirlaz via ccca3ba May 17, 2021 23:21

keryell previously approved these changes May 18, 2021

View reviewed changes

vladimirlaz previously approved these changes May 18, 2021

View reviewed changes

Change fpga_reg to use a deprecation warning for old APIs

079f31a

ajaykumarkannan dismissed stale reviews from vladimirlaz and keryell via 079f31a May 25, 2021 14:56

kbsmith-intel approved these changes May 25, 2021

View reviewed changes

sycl/include/CL/sycl/INTEL/fpga_reg.hpp Show resolved Hide resolved

MrSidims approved these changes Jun 8, 2021

View reviewed changes

vladimirlaz approved these changes Jun 29, 2021

View reviewed changes

bader merged commit b4c322a into intel:sycl Jun 29, 2021

ajaykumarkannan deleted the fpga_reg_update branch June 29, 2021 17:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SYCL] Allow fpga_reg only for PODs and Trivially-copyable structs #3643

[SYCL] Allow fpga_reg only for PODs and Trivially-copyable structs #3643

ajaykumarkannan commented Apr 28, 2021

bader commented Apr 29, 2021

mkinsner commented Apr 29, 2021

jessicadavies-intel commented Apr 29, 2021

ajaykumarkannan commented Apr 29, 2021

ajaykumarkannan commented Apr 29, 2021 •

edited

Loading

ajaykumarkannan commented Apr 29, 2021

ajaykumarkannan commented Apr 29, 2021 •

edited

Loading

bader commented May 11, 2021

pvchupin commented May 11, 2021

vladimirlaz left a comment

mkinsner commented May 13, 2021

keryell May 18, 2021

ajaykumarkannan commented May 25, 2021

kbsmith-intel left a comment

ajaykumarkannan commented Jun 8, 2021

[SYCL] Allow fpga_reg only for PODs and Trivially-copyable structs #3643

[SYCL] Allow fpga_reg only for PODs and Trivially-copyable structs #3643

Conversation

ajaykumarkannan commented Apr 28, 2021

bader commented Apr 29, 2021

mkinsner commented Apr 29, 2021

jessicadavies-intel commented Apr 29, 2021

ajaykumarkannan commented Apr 29, 2021

ajaykumarkannan commented Apr 29, 2021 • edited Loading

ajaykumarkannan commented Apr 29, 2021

ajaykumarkannan commented Apr 29, 2021 • edited Loading

bader commented May 11, 2021

pvchupin commented May 11, 2021

vladimirlaz left a comment

Choose a reason for hiding this comment

mkinsner commented May 13, 2021

keryell May 18, 2021

Choose a reason for hiding this comment

ajaykumarkannan commented May 25, 2021

kbsmith-intel left a comment

Choose a reason for hiding this comment

ajaykumarkannan commented Jun 8, 2021

ajaykumarkannan commented Apr 29, 2021 •

edited

Loading

ajaykumarkannan commented Apr 29, 2021 •

edited

Loading