[Openmp-commits] [PATCH] D137607: Add interop support for OpenMP AMD GPU plugin

[Jisheng Zhao via Phabricator via Openmp-commits]

jz10 updated this revision to Diff 479522.
jz10 added a comment.

Thanks Johannes, here are replies

1. "I'm now very confused what public APIs exist and which ones are actually needed/used. We should have a single one, but I'm fairly sure that's not the case, right? Why not. I don't understand the sentence I quoted. Maybe elaborate a little more if you think we should have more than one external plugin API."

Sorry that brings this confusion. What I was trying to say is plugins/cuda has "__tgt_rtl_init_async_info" and "__tgt_rtl_init_device_info", so I added those two APIs to plugins/amdgpu accordingly.  To my understanding of your suggestion (maybe wrong) , is to get rid of  "__tgt_rtl_init_device_info" could be removed since it is not invoked at somewhere else

2. "There is an assert followed by a check, that seems wrong. Either it's a pre-condition or it's something we want to handle gracefully.

A nullptr queue is not an error code. It is a perfectly valid state meaning there is no outstanding async work. Check the CUDA impl, it will just return OFFLOAD_FAIL if the DeviceId is invalid, do the same.
There is an assert followed by a check, that seems wrong. Either it's a pre-condition or it's something we want to handle gracefully."

fixed

3. "We are actually not using hip at all. There is no backend or vendor part that is hip. We use HSA. Add a new enum value for both."

added a new enum as 'amdhsa', since 'hsa' has been used for namespace

4. "Why the ( )?"

fixed, but there are some other legacy code that still use ("...")

5. "Clang format this file please"

fixed


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137607/new/

https://reviews.llvm.org/D137607

Files:
  openmp/libomptarget/include/interop.h
  openmp/libomptarget/include/omptarget.h
  openmp/libomptarget/include/omptargetplugin.h
  openmp/libomptarget/include/rtl.h
  openmp/libomptarget/plugins/amdgpu/src/rtl.cpp
  openmp/libomptarget/plugins/cuda/src/rtl.cpp
  openmp/libomptarget/src/interop.cpp
  openmp/libomptarget/src/rtl.cpp
  openmp/libomptarget/test/api/omp_interop_amdgpu.c

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D137607.479522.patch
Type: text/x-patch
Size: 13054 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/openmp-commits/attachments/20221202/54fe7a73/attachment-0001.bin>