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

Jon Chesterfield via Phabricator via Openmp-commits openmp-commits at lists.llvm.org
Fri Dec 17 09:00:58 PST 2021

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.

  rG LLVM Github Monorepo



More information about the Openmp-commits mailing list