[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:118
+  void *Identifier;
+};
+
----------------
tianshilei1992 wrote:
> jdoerfert wrote:
> > I don't like `Identifier` as name. Maybe execution queue? Or the name we used for streams before? Also, add a comment what it is and why it is a void *
> The basic idea is, this structure is device independent. For CUDA device, we use it as `CUstream` so yes, we can call it stream. However, we don't know how it can be used in other platforms, and that's why I use a "weird" word `Identifier` and a `void *` here.
We need a better word and documentation of the member. Also initialize it here


================
Comment at: openmp/libomptarget/plugins/cuda/src/rtl.cpp:320
+    AsyncInfo->Identifier = DeviceInfo.getNextStream(Id);
+  }
+
----------------
tianshilei1992 wrote:
> jdoerfert wrote:
> > Nit: I know the style in here is mixed but I would remove the braces around single statements, especially return.
> Speaking of this, is it required in LLVM that single-statement should not be wrapped into braces? It is often good practice to have these braces even just with one statement.
Coding style says no braces but the runtime is unfortunately not in accordance...


================
Comment at: openmp/libomptarget/plugins/cuda/src/rtl.cpp:913
+
+  auto Stream = reinterpret_cast<CUstream>(async_info->Identifier);
+  CUresult Err = cuStreamSynchronize(Stream);
----------------
tianshilei1992 wrote:
> jdoerfert wrote:
> > tianshilei1992 wrote:
> > > jdoerfert wrote:
> > > > Why reinterpret_Cast? Is a CUstream a pointer? If not, why do we move objects around?
> > > Yep.
> > > ```
> > > typedef struct CUstream_st *CUstream;                     /**< CUDA stream */
> > > ```
> > then we should be able to just use a static cast, shouldn't we? Maybe also add a static_assert just to be safe. Also other places.
> Correct me if I'm wrong. Compiler will generate code for the cast if using `static_cast`, but it will not for `reinterpret_cast`. My thought is, since it is already in CUDA plugin, we can make sure that this `void *` is actually a `CUstream` so we can directly reinterpret it to avoid one cast instruction.
It could but not for such a pointer 2 pointer cast on a "normal" architecture. It's just that I don't see reinterpret_cast often, almost 1:4 against static_cast, but I'm fine with leaving it.


================
Comment at: openmp/libomptarget/src/device.h:184
+  int32_t data_retrieve(void *HstPtrBegin, void *TgtPtrBegin, int64_t Size,
+                        __tgt_async_info *AsyncInfoPtr);
+
----------------
tianshilei1992 wrote:
> jdoerfert wrote:
> > Remove the old versions if we don't expose them to the outside. Let's not accumulate clutter. Similarly for other functions.
> Do we need to make `AsyncInfoPtr` with default value?
Not if we can modify all callers (which we can if the symbols is not exported). If we can't we should add the new API calls as you have.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77005