llvm icon indicating copy to clipboard operation
llvm copied to clipboard
verified publisher icon intel

[SYCL][AOT][NVPTX][AMD][New offloading model] Add AOT support for NVIDIA and AMD backend in the clang-linker-wrapper for SYCL offloading

Open asudarsa opened this issue 1 year ago • 4 comments

This PR adds third-party (nvptx, amd) AOT support for SYCL offloading in the clang-linker-wrapper tool. Following list of changes are found;

  1. Code inside clang-linker-wrapper has been refactored to get it ready for non-SPIRV backends for SYCL offloading. Main changes are inside linkAndWrapDeviceFiles. Explanations are added as PR comments.
  2. A new clang-linker-wrapper option to pass additional device library files for NVPTX backend has been added
  3. An additional check to emit error when device library files are unexpectedly empty has been added
  4. New tests have been added to test the compilation flow for the new changes and also existing tests have been modified to react to the additional check correctly.

E2E tests and more extensive driver tests will be added in the next stage.

Thanks

asudarsa avatar Jun 28 '24 05:06 asudarsa

  1. Explanations are added as PR comments.

Why not code comments?

bader avatar Jun 28 '24 18:06 bader

  1. Explanations are added as PR comments.

Why not code comments?

These comments are meant for reviewers who have been involved in the incremental development of this feature support. It explains what existed in the previous version and how things evolved in this version as I added AOT support for third party hardware. These comments do not make much sense for users/developers looking at the final version of the code. Hope that is agreeable.

Thanks

asudarsa avatar Jun 28 '24 19:06 asudarsa

overall - looks OK to me. Given there are still some changes needed to improve the testing area, I'll have @hchilama take another look when the bits are ready as she is covering for me while I'm out.

mdtoguchi avatar Jun 29 '24 00:06 mdtoguchi

overall - looks OK to me. Given there are still some changes needed to improve the testing area, I'll have @hchilama take another look when the bits are ready as she is covering for me while I'm out.

I have made changes to driver tests that I have added over time (mainly for new offloading model testing) to remove all .o files and am now generating them on the fly. @hchilama, can you please take a look when convenient?

Thanks

asudarsa avatar Jul 01 '24 02:07 asudarsa

Hi @bader

Thanks so very much for the detailed review. I have learnt a bunch about writing better tests here. I have addressed all the comments except the one related to 'doxygen' documentation which will be addressed in an upcoming patch.

Sincerely

asudarsa avatar Jul 01 '24 22:07 asudarsa