[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:
> 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
Sure. Do you have any suggestion for the name?


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


================
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:
> 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.
Got you. Will do that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77005