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

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert added a comment.

In D137607#3927376 <https://reviews.llvm.org/D137607#3927376>, @jz10 wrote:

> 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.

So, keeping the original flow is not as important as designing it properly.
Right now, if the device is not ready, you still call into the device RTL.
Either we can always do that, and therefore can always call init_device_info to initialize the thing in the plugin, or we cannot always do that, and should consequently not call get_driver_type if the device is not ready.
To determine which, we need to look at the "non-ready" case and what that means. If the latter is the case, we can initialize the interop type with "unknown" instead of CUDA.
Does that make sense?



================
Comment at: openmp/libomptarget/plugins/amdgpu/src/rtl.cpp:2708
+
+  return OFFLOAD_FAIL;
+}
----------------
When I said, no return after else, I mean, remove the "else" not the "return". You should take a look at the LLVM coding style guide.

```
if (...) {
  ...
  return A;
} else {
  ...
}
return B;
```
should be

```
if (...) {
  ...
  return A;
}
...
return B;
```
You still have the former pattern in your code and you now again have the complicated case first. It's easier to read if you have
```
if (...) {
  // short
  return;
}
//long
```
than

```
if (...) {
  // long
} else {
  // short
  return;
}
```


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

https://reviews.llvm.org/D137607