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

[Shilei Tian via Phabricator via Openmp-commits]

tianshilei1992 added inline comments.


================
Comment at: openmp/libomptarget/include/omptarget.h:118
+  void *Identifier;
+};
+
----------------
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.


================
Comment at: openmp/libomptarget/plugins/cuda/src/rtl.cpp:320
+    AsyncInfo->Identifier = DeviceInfo.getNextStream(Id);
+  }
+
----------------
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.


================
Comment at: openmp/libomptarget/plugins/cuda/src/rtl.cpp:913
+
+  auto Stream = reinterpret_cast<CUstream>(async_info->Identifier);
+  CUresult Err = cuStreamSynchronize(Stream);
----------------
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.


================
Comment at: openmp/libomptarget/src/device.h:184
+  int32_t data_retrieve(void *HstPtrBegin, void *TgtPtrBegin, int64_t Size,
+                        __tgt_async_info *AsyncInfoPtr);
+
----------------
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?


================
Comment at: openmp/libomptarget/src/omptarget.cpp:646
+  __tgt_async_info AsynInfo;
+  AsynInfo.Identifier = nullptr;
+
----------------
jdoerfert wrote:
> Move this to the struct definition. It's C++ after all.
Yep. Will do that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77005