[Openmp-commits] [PATCH] D115823: [openmp][libomptarget] Introduce and call createAsyncInfo

[Ye Luo via Phabricator via Openmp-commits]

ye-luo added a comment.

I never liked init-if-passed-nullptr and prefer this explicit approach.



================
Comment at: openmp/libomptarget/plugins/amdgpu/src/rtl.cpp:2207
 
+int32_t __tgt_rtl_create_data_async(int, __tgt_async_info *AsyncInfo) {
+  initAsyncInfo(AsyncInfo);
----------------
Should the name be `__tgt_rtl_create_async_info`?


================
Comment at: openmp/libomptarget/plugins/cuda/src/rtl.cpp:944
 
+  CUstream getStream(const int DeviceId, __tgt_async_info *AsyncInfo) const {
+    assert(AsyncInfo && "AsyncInfo is nullptr");
----------------
getStream should remain private. It leaks CUstream if put public.
getStream doesn't recreate AsyncInfo->Queue if it is not nullptr.
I would expect an error if createAsyncInfo.
So Please add a new public function instead of reusing getStream.


================
Comment at: openmp/libomptarget/plugins/cuda/src/rtl.cpp:1453
 
+int32_t __tgt_rtl_create_data_async(int DeviceId, __tgt_async_info *AsyncInfo) {
+  DeviceRTL.getStream(DeviceId, AsyncInfo);
----------------
Should the name be `__tgt_rtl_create_async_info`?


================
Comment at: openmp/libomptarget/src/device.cpp:456
 
-  if (!AsyncInfo || !RTL->data_submit_async || !RTL->synchronize)
     return RTL->data_submit(RTLDeviceID, TgtPtrBegin, HstPtrBegin, Size);
----------------
I'm wondering how is `!AsyncInfo ` is compiled. AsyncInfoTy doesn't seem to have "operator bool"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115823