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

[Jon Chesterfield via Phabricator via Openmp-commits]

JonChesterfield planned changes to this revision.
JonChesterfield added inline comments.


================
Comment at: openmp/libomptarget/plugins/amdgpu/src/rtl.cpp:2207
 
+int32_t __tgt_rtl_create_data_async(int, __tgt_async_info *AsyncInfo) {
+  initAsyncInfo(AsyncInfo);
----------------
ye-luo wrote:
> Should the name be `__tgt_rtl_create_async_info`?
yeah, it should. ouch. trying to think of a way to verify things like that (another patch had the wrong prototype, which meant it defined an unrelated function with c++ mangling that got ignored)


================
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");
----------------
ye-luo wrote:
> jdoerfert wrote:
> > ye-luo wrote:
> > > 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.
> > I think my asyncmalloc patch makes it public too. 
> > I think my asyncmalloc patch makes it public too. 
> 
> Then my concern applies to your patch as well. LOL
seems reasonable to keep the CUstream hidden, will do so


================
Comment at: openmp/libomptarget/src/device.cpp:456
 
-  if (!AsyncInfo || !RTL->data_submit_async || !RTL->synchronize)
     return RTL->data_submit(RTLDeviceID, TgtPtrBegin, HstPtrBegin, Size);
----------------
ye-luo wrote:
> I'm wondering how is `!AsyncInfo ` is compiled. AsyncInfoTy doesn't seem to have "operator bool"
Interesting yeah, a brief trial with godbolt shows both gcc and clang rejecting similar constructs. Quirk of compiler flags in use perhaps. I'm assuming it was compiled to 'true' here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115823