This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP] Move synchronization into `__tgt_async_info`
ClosedPublic

Authored by jdoerfert on Feb 10 2021, 10:35 AM.

Details

Summary

The AsyncInfo should be passed everywhere and it should offer a way to
ensure synchronization, given a libomptarget Device.

This replaces D96431.

Diff Detail

Event Timeline

jdoerfert created this revision.Feb 10 2021, 10:35 AM
jdoerfert requested review of this revision.Feb 10 2021, 10:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 10 2021, 10:35 AM
Herald added a subscriber: sstefan1. · View Herald Transcript
openmp/libomptarget/include/omptarget.h
126–128

I suggest we wrap this info into another class, and bind the device object such that we will make sure that the async info object will not be synced by a wrong device.

class AsyncInfo {
  __tgt_async_info TheInfo;
  DeviceTy &Device;
public:
  int synchronize();
};

Then __tgt_async_info is only used when we're communicating with plugin. AsyncInfo is used across the whole libomptarget.

Introduced wrapper

tianshilei1992 added inline comments.Feb 10 2021, 2:18 PM
openmp/libomptarget/include/omptarget.h
136

Not sure the wrapper has a bonus to fix issues mentioned in D96431.

openmp/libomptarget/src/omptarget.cpp
1144

The last argument of DeviceTy::submitData is __tgt_async_info *. I'm thinking we can pass AsyncInfoTy & here and all conversion is done in DeviceTy such that all interfaces in libomptarget use same thing.

jdoerfert added inline comments.Feb 10 2021, 3:03 PM
openmp/libomptarget/include/omptarget.h
126–128

That's what is happening after this patch and a second to replace the remaining uses of __tgt_async_info in the libomptarget layer. Already with this patch synchronize will work on the right device.

136

I don't understand.

tianshilei1992 added inline comments.Feb 10 2021, 4:45 PM
openmp/libomptarget/include/omptarget.h
136

If we have event, AsyncInfoTy's lifetime can still be out of its scope.

jdoerfert added inline comments.Feb 10 2021, 4:47 PM
openmp/libomptarget/include/omptarget.h
136

Let's cross that bridge when we get events? I was assuming we copy the __tgt_async_info into the appropriate locations of the runtime and null it in the AsyncInfoTy object. Or something along those lines.

tianshilei1992 added inline comments.Feb 10 2021, 4:54 PM
openmp/libomptarget/include/omptarget.h
136

That should be fine. Actually we cannot use AsyncInfoTy in libomp because it's a C library. If it is a C++ library, we also don't need to worry about that because we can use smart pointer then,

openmp/libomptarget/src/omptarget.cpp
647

This will cause synchronize to be called twice: one here, and another when AsyncInfo is destroyed. For now __tgt_rtl_synchronize is not designed to be called on a same object twice because it will assert(async_info_ptr->Queue), and every time we call synchronize, Queue will be set to nullptr.

jdoerfert added inline comments.Feb 16 2021, 11:30 AM
openmp/libomptarget/src/omptarget.cpp
647

AsyncInfoTy::synchronize() checks the Queue and only forwards the call if it is not null. Calling it twice is OK.

This revision is now accepted and ready to land.Feb 16 2021, 11:58 AM
This revision was landed with ongoing or failed builds.Feb 16 2021, 1:38 PM
This revision was automatically updated to reflect the committed changes.