Conversation
googletest: release-1.11.0 hlslib: v1.4.1 clang: 12.0.1 OpenCL: 2.0 IntelSDK: 20.4.0
|
Hello, we updated SMI so we can better integrate it into our project. We would be glad if you can review this. Best Regards |
TizianoDeMatteis
left a comment
TizianoDeMatteis
left a comment
There was a problem hiding this comment.
Hello,
thanks for your PR!
I've left some comments, in particular the changes to the CMake file are not clear to me.
Have you double-checked that tests still work? I was not able to reproduce them (but maybe something is wrong with my configuration)
| set(FPGA_REPORT_TARGET ${TARGET_NAME}_${KERNEL_NAME}_aoc_report) | ||
| add_custom_target(${FPGA_REPORT_TARGET} | ||
| COMMAND ${IntelFPGAOpenCL_AOC} ${AOC_COMMAND} ${FPGA_SRC_FILES} -rtl -report | ||
| COMMAND ${IntelFPGAOpenCL_AOC} ${KERNEL_GENERATED_PATH} -I${KERNEL_BIN_DIR} ${AOC_COMMAND} -rtl -report |
There was a problem hiding this comment.
Why is this change necessary?
There was a problem hiding this comment.
The new compiler version is not able to compile two cl files together, so we switched to compiling only the kernel file and then include the smi_generated_device.cl from there.
CMakeLists.txt
Outdated
| function(fpga_target TARGET_NAME HOST_SOURCE KERNEL GENERATE_KERNEL) | ||
|
|
||
| set(FPGA_SOURCES) # list of transformed user device files (one per program) | ||
| set(PGA_SOURCES) # list of transformed user device files (one per program) |
| for (int i = num_kernels - 1; i >= 0; i--) | ||
| { | ||
| queues[i].enqueueTask(kernels[i]); | ||
| queues[i].enqueueNDRangeKernel(kernels[i], cl::NullRange, cl::NDRange(1)); |
There was a problem hiding this comment.
Is there any advantage in using eneuqueNDRangeKernel?
There was a problem hiding this comment.
enqueueTask is no more available in cl2
| #include <utils/utils.hpp> | ||
| #define TILE_SIZE 128 //define this as used in the opencl kernel | ||
|
|
||
| #if !defined(CL_CHANNEL_1_INTELFPGA) |
There was a problem hiding this comment.
I think that this should not be necessary
|
|
||
| */ | ||
|
|
||
| #include "smi_generated_device.cl" |
There was a problem hiding this comment.
These include (and the ones in the successive file) should not be necessary with the old CMakeList
|
Thanks for the fast review. Currently i am not able to verify the tests on the master branch |
the new intel SDK introduced an vendor string only for the emulation device which is not supported in hlslib. because the vendor string is also used in initOpenCL directly i introduced it into SMI, but in perspective it should be integrated into hlsblib and used from there
2 participants
googletest: release-1.11.0
hlslib: v1.4.1
clang: 12.0.1
OpenCL: 2.0
IntelSDK: 20.4.0