search

oneapi-src / oneAPI-samples

Samples for Intel® oneAPI Toolkits
https://oneapi-src.github.io/oneAPI-samples/
MIT License
922 stars 681 forks source link

Update host pipe tests for FPGA emulator #2461

Closed qichaogu closed 2 weeks ago

qichaogu commented 3 weeks ago

Existing Sample Changes

Description

When a host pipe on FPGA emulator does not specify the depth, the depth will be 1 by default. For tests that write a number of data packets to a pipe, the pipe needs to specify its depth large enough to store data packets without any being read out.

Type of change

Please delete options that are not relevant. Add a 'X' to the one that is applicable.

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

qichaogu commented 3 weeks ago

Ping @whitepau for review.

qichaogu commented 3 weeks ago

Will this change be made unnecessary by fixing https://hsdes.intel.com/appstore/article/#/14019216094?

I have no access to https://hsdes.intel.com/appstore/article/#/14019216094.

jimmytwei commented 3 weeks ago

@qichaogu @whitepau Let me know when I can merge this PR. Thanks.

justin-rosner commented 3 weeks ago

Will this change be made unnecessary by fixing https://hsdes.intel.com/appstore/article/#/14019216094?

This PR makes changes that hurt readability of this tutorial without explaining them. Please make sure that code changes are minimal (the specific problem is in hostpipes.cpp) and that they are explained in the Readme.

If this is a general best practice for FPGA, we should update this tutorial as well: https://github.com/oneapi-src/oneAPI-samples/tree/master/DirectProgramming/C%2B%2BSYCL_FPGA/Tutorials/Features/hls_flow_interfaces/streaming_data_interfaces

Would changing the CL_CONFIG_CHANNEL_DEPTH_EMULATION_MODE environment variable also give us the desired result without having to make code sample changes?

whitepau commented 3 weeks ago

@qichaogu @whitepau Let me know when I can merge this PR. Thanks.

I do not like the use model this proposes (namely; that users have to write different code for the emulator vs the simulator).

@qichaogu please consider @justin-rosner's suggestion.

Can you two please work together to ensure that this change is not necessary if the change documented in the HSD ES case is implemented? Since you don't have access to the case here is the summary:

It proposes to give emulation host pipes 'infinite capacity'. A host pipe really has two 'capacities': the depth of the FIFO in the device (which is set by the pipedepth property), and the size of the buffer in host memory. We can grow the host buffer as large as we want at runtime, since we never actually specify how big this buffer is.

justin-rosner commented 2 weeks ago

@qichaogu @whitepau Let me know when I can merge this PR. Thanks.

I do not like the use model this proposes (namely; that users have to write different code for the emulator vs the simulator).

@qichaogu please consider @justin-rosner's suggestion.

Can you two please work together to ensure that this change is not necessary if the change documented in the HSD ES case is implemented? Since you don't have access to the case here is the summary:

It proposes to give emulation host pipes 'infinite capacity'. A host pipe really has two 'capacities': the depth of the FIFO in the device (which is set by the pipedepth property), and the size of the buffer in host memory. We can grow the host buffer as large as we want at runtime, since we never actually specify how big this buffer is.

The case I'm currently working on is to do with the buffer capacity in simulation, not emulation, but yes. I think it would be a good idea to sync up to make sure that emulation behaviour and simulation behaviour match

qichaogu commented 2 weeks ago

Will this change be made unnecessary by fixing https://hsdes.intel.com/appstore/article/#/14019216094? This PR makes changes that hurt readability of this tutorial without explaining them. Please make sure that code changes are minimal (the specific problem is in hostpipes.cpp) and that they are explained in the Readme. If this is a general best practice for FPGA, we should update this tutorial as well: https://github.com/oneapi-src/oneAPI-samples/tree/master/DirectProgramming/C%2B%2BSYCL_FPGA/Tutorials/Features/hls_flow_interfaces/streaming_data_interfaces

Would changing the CL_CONFIG_CHANNEL_DEPTH_EMULATION_MODE environment variable also give us the desired result without having to make code sample changes?

Yes, I'm working on fix the mode control CL_CONFIG_CHANNEL_DEPTH_EMULATION_MODE. So there is no need to modify the sample code here. I'll close the PR. @justin-rosner @whitepau @jimmytwei Thanks for all your comments.