[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
openmp-commits at lists.llvm.org
Mon Mar 30 20:13:41 PDT 2020
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);
> jdoerfert wrote:
> > Why reinterpret_Cast? Is a CUstream a pointer? If not, why do we move objects around?
> 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.
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
More information about the Openmp-commits