[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;
+};
+
----------------
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 *


================
Comment at: openmp/libomptarget/plugins/cuda/src/rtl.cpp:320
+    AsyncInfo->Identifier = DeviceInfo.getNextStream(Id);
+  }
+
----------------
Nit: I know the style in here is mixed but I would remove the braces around single statements, especially return.


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


================
Comment at: openmp/libomptarget/src/device.h:184
+  int32_t data_retrieve(void *HstPtrBegin, void *TgtPtrBegin, int64_t Size,
+                        __tgt_async_info *AsyncInfoPtr);
+
----------------
Remove the old versions if we don't expose them to the outside. Let's not accumulate clutter. Similarly for other functions.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77005