This is an archive of the discontinued LLVM Phabricator instance.

[openmp][libomptarget] Introduce and call createAsyncInfo
Changes PlannedPublic

Authored by JonChesterfield on Dec 15 2021, 12:14 PM.

Details

Summary

At present, every async function is responsible for noticing it has
been passed a nullptr for async_info and initializing it. With this change,
a new api function is called to initialize async_info within libomptarget.

A subsequent change can then only call async functions when that construction
succeeded, at which point the init-if-passed-nullptr logic from the plugins
can be replaced with asserts.

Couple of typo fixes noticed while implementing. Builds on D96438.

Diff Detail

Event Timeline

JonChesterfield requested review of this revision.Dec 15 2021, 12:14 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 15 2021, 12:14 PM
  • clang-format
openmp/libomptarget/include/omptargetplugin.h
76

API is redundant at present, but it matches the other functions and we might want to return more detailed information than success/fail later

openmp/libomptarget/plugins/cuda/src/rtl.cpp
470–471

moved this to make it public

  • delete debugging cruft
This revision is now accepted and ready to land.Dec 16 2021, 8:17 AM

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

openmp/libomptarget/plugins/amdgpu/src/rtl.cpp
2159

Should the name be __tgt_rtl_create_async_info?

openmp/libomptarget/plugins/cuda/src/rtl.cpp
944

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.

1453

Should the name be __tgt_rtl_create_async_info?

openmp/libomptarget/src/device.cpp
456

I'm wondering how is !AsyncInfo is compiled. AsyncInfoTy doesn't seem to have "operator bool"

jdoerfert added inline comments.Dec 16 2021, 10:26 PM
openmp/libomptarget/plugins/cuda/src/rtl.cpp
944

I think my asyncmalloc patch makes it public too.

ye-luo added inline comments.Dec 16 2021, 10:31 PM
openmp/libomptarget/plugins/cuda/src/rtl.cpp
944

I think my asyncmalloc patch makes it public too.

Then my concern applies to your patch as well. LOL

JonChesterfield planned changes to this revision.Dec 17 2021, 9:00 AM
JonChesterfield added inline comments.
openmp/libomptarget/plugins/amdgpu/src/rtl.cpp
2159

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)

openmp/libomptarget/plugins/cuda/src/rtl.cpp
944

seems reasonable to keep the CUstream hidden, will do so

openmp/libomptarget/src/device.cpp
456

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.