[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 475433.
jz10 added a comment.

Thanks Johannes, I addressed your comments as follows:

1. "Why don't we "just" return the int32_t? The indirection seems counterintuitive to me. Also, "DriverInfo" is not really descriptive. Can you specify what it returns, wrt. standard defined structs etc."

I revised the interface and API name

2. "Please don't cast to "int". If anything, pick a explicit size."

fixed

3. "No else after return (is what I meant). Also other places."

fixed

4. "This seems odd. Is the runtime and the vendor the same thing for us?"

this is the test code that was not removed, fixed

5. Do you know it's null or allocated here?

fixed, see below

6. "Why do we need this new function if we call init device info afterwards anyway? It seems we now have two functions we always call in sequence to get two numbers out of the plugin. If so, we should merge them."

I revised the implementation of "__tgt_interop_init" based on its original workflow.
The original workflow for "__tgt_interop_init" is :
i. handle task dependencies
ii. create omp_interop_val_t, but the interop type was constantly set as CUDA (this is why we need to retrieve the driver type from plugin)
iii. check if the device is ready, if not, set the error string but preserve the omp_interop_val_t ;
iv. call init_device_info to setup the Context and Device fields for __tgt_device_info object, if this failed, then destroy omp_interop_val_t;
v. if the interop type is kmp_interop_type_tasksync, then call init_async_info to setup the async Queue for  __tgt_async_info object, if this failed, then destroy omp_interop_val_t.

based on these steps, we currently don't merge the omp_interop_val_t object creation together with init_device_info,

7. "Why is the ready check happening after the rest, could you explain the problem here?"

fixed

8. "Where is test case"

I added the test case as openmp/libomptarget/test/api/test_interop_amdgpu.c


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.475433.patch
Type: text/x-patch
Size: 11574 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/openmp-commits/attachments/20221115/a899df1d/attachment-0001.bin>