Skip to content
This repository has been archived by the owner on May 13, 2024. It is now read-only.

Add oneAPI DPC++ ESIMD extention as a new backend for T2S on GPU #21

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

tanzelin430
Copy link

No description provided.

Copy link
Collaborator

@ronghongbo ronghongbo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Zelin, thanks for the great efforts. Overall, it looks good. There are many minor places to fix, though:

  1. OneAPI and DPC: we have both terms used in many places due to historical reasons. You may help clean up this mess. I would suggest you to change them into something like "DPC_GPU" and "DPC_FPGA", or "OneAPI_GPU" and "OneAPI_FPGA". But you can leave this as the next step, though.
  2. Halide/performance: what is the purpose of this directory? remove it if not needed
  3. Halide/preprocessor: same question as above
  4. Codegen_GPU_Host.cpp: 120: I would suggest to add "CM", "DPC/OneAPI_FPGA", "DPC/OneAPI_GPU" features to Halide/src/Target.h and check against them.
  5. Halide/src/CodeGen_LLVM.cpp: The debugging line is useless now after you made things work
  6. Halide/src/Module.h: 43: again, we have the mix-use of "DPC" and "OneAPI". They are the same and we should keep only one of them.
  7. Target.cpp: 666 and 669: Add something like WITH_DPC in Halide/Makefile, and change these lines correspondingly.
  8. Target.cpp: 690-694: if the else statement is not useful any more, remove it.
  9. Pipeline.cpp: make { after a statement so that the coding style is consistent with the rest of the system
  10. install-tool.sh: 24: can a common user uses "apt install"? I guess users do not necessarily have the root privilege to do that. Since python is popular and used everywhere, I would suggest to not install python. Instead, in README, just list python (2x or 3x) as a prerequisite.
  11. setenv.sh: keep the useful changes, and remove the dead ones.
  12. preprocessor: we need to move it to t2s/src, and also clean up the file names (like post.t2s.......) Currently Abe makes file names too long). We should make the file names consistent with the workflow.png shows
  13. preprocessor/sample: (1) the several samples have many generated files. They should not be checked in. (2) oneapi-target-4-parameters.h should be renamed into const-parameters.h? (3) HalideBuffer., buffer_t., HalideRuntime., runtime_internal. should all be removed. They can be found from Halide/src, Halide/src/runtime, etc. So in a command line, you can use "-I ..." to set include paths to include .h files, or add path_to_a_cpp_file/something.cpp, to make things work without such copies. (3) pipe_array* files are copied from https://oneapi.team/tattle/oneAPI-samples/-/tree/9d8b94a38f2a98042cf933adfb91ec1da3d5ad51/DirectProgramming/DPC++FPGA/Tutorials/DesignPatterns/pipe_array/src. Please add some command lines in install-tool.sh to achieve this effect, instead of hard copies. (4) Sample03/perf.cpp: is it still useful? (5) util.h can be included from t2s/tests/correctness/util. No need for another copy.
  14. tests/performance/capsule.cpp: it does not seem you have made any real change. Recover the original files if the only difference is format. I saw some other files having similar issues. You would like to minimize the number of files in your check in
  15. performance/capsule/const-parameters.h: why changing the parameters? That would invalidate the performance numbers we report.
  16. performance/gemm/const-parameters.h: same question as above
  17. performance/gemm/gemm-run-gpu-oneapi.cpp: is this file useful?

Please clean up the files based on the feedback, and let me take a look again. Thanks!

@ronghongbo
Copy link
Collaborator

Xiaochen has merged your changes into his fork, and will push to main repo in future together with his changes.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants