[Openmp-commits] [PATCH] D77005: [OpenMP] Optimized stream selection by scheduling data mapping for the same target region into a same stream

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert added inline comments.


================
Comment at: openmp/libomptarget/include/omptarget.h:119
+};
+
 #ifdef __cplusplus
----------------
tianshilei1992 wrote:
> lildmh wrote:
> > Do you need a clang patch to generate code for this structure and new APIs?
> Currently no. But of course this information can be used by Clang in the future if needed.
We'll target the new API also/mainly by rewriting old API calls in OpenMPOpt


================
Comment at: openmp/libomptarget/plugins/cuda/src/rtl.cpp:909
+  // If there is no any information, just return
+  if (!async_info->Identifier) {
+    return OFFLOAD_SUCCESS;
----------------
tianshilei1992 wrote:
> lildmh wrote:
> > It seems no synchronization by default, not aligned with the original behavior
> The assumption here is: if this function is called at the end of `target` function, `Identifier` must be a valid pointer because anyhow during kernel launch it is assigned with a valid `CUstream`.
It seems wrong to call this with `Identifier = nullptr`, doesn't it? If so we should make it an assert.


================
Comment at: openmp/libomptarget/plugins/cuda/src/rtl.cpp:913
+
+  auto Stream = reinterpret_cast<CUstream>(async_info->Identifier);
+  CUresult Err = cuStreamSynchronize(Stream);
----------------
Why reinterpret_Cast? Is a CUstream a pointer? If not, why do we move objects around?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77005