Page MenuHomePhabricator

Runtime for Interop directive
Needs ReviewPublic

Authored by sriharikrishna on Jul 23 2021, 8:43 AM.

Details

Summary

This implements the runtime portion of the interop directive.
It expects the frontend and IRBuilder portions to be in place
for proper execution. It currently works only for GPUs
and has several TODOs that should be addressed going forward.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
sriharikrishna requested review of this revision.Jul 23 2021, 8:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 23 2021, 8:43 AM

You should not be using kmpc in libomptarget

Looks like openmp/runtime/src/dllexports is edited by mistake. Added symbols are defined nowhere that causes build failure on Windows.

tianshilei1992 added inline comments.
openmp/libomptarget/include/interop.h
20

Probably not an option.

openmp/libomptarget/include/omptarget.h
21

Why is that?

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

I think these series of changes regarding SPMD_GENERIC has nothing to do with interop. Please make a separate patch for them.

1137–1138
openmp/libomptarget/src/CMakeLists.txt
21

order please

openmp/libomptarget/src/exports
49

Like @RaviNarayanaswamy said, better to rename them to __tgt_xxx.

openmp/libomptarget/src/interop.cpp
14

There are some conventions in current libomptarget.

  1. If a function is internal, use LLVM code style.
  2. If a function is exported and exposed to compiler, it should be extern "C" and use code style similar to existing functions whose name prefix with __tgt.

So basically, if these functions are only visible to this file, please format it with LLVM code style, and use anonymous name space.

openmp/libomptarget/src/private.h
117

If this function is not defined in libomp, please don't use __kmpc.

There are a lot of minor things that should be easily addressable.

openmp/libomptarget/include/interop.h
2

We need the llvm license header, copied from another file.

66

put the entire file content (after macros) into a single extern C.

110

The functions above need definitions in libomp and libomptarget.
For the former we need to edit the following two files, look for "get_device_num" (or "GET_DEVICE_NUM") in both
to see how to define it weak:
openmp/runtime/src/kmp_ftn_os.h
openmp/runtime/src/kmp_ftn_entry.h
just return 0 or nullptr for all of them in libomp.

In libomptarget already have the definitions and exports.

161

Remove the asserts.

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

This is a rebase problem. These changes are already upstream but somehow they managed to get into this commit/revision. Rebase locally and this should hopefully be gone.

openmp/libomptarget/src/exports
1

undo this.

openmp/libomptarget/src/interop.cpp
46

We nee a switch and return all values in Table 2.1 (https://www.openmp.org/wp-content/uploads/OpenMP-API-Additional-Definitions-2-0.pdf) or "unknown" as default.

178

don't use references in this code with C bindings. Use a ** instead and change left-hand-side uses to *interop_ptr. This will then also match D105876 where all 3 functions are declared to take a void**. That means _use below should take a void** as well.

192

This assertion seems nonsensical, just return I guess.

205

Move the wait stuff to the beginning.

217

Again
interop_ptr = omp_interop_none;

openmp/libomptarget/src/private.h
117

It is in libomp, this is a forward decl. All good here.

openmp/runtime/src/dllexports
582

Maybe remove all these for now. That should also allow to not define omp_get_interop_XXX in libomp for now and it will just fail if we do not link libomptarget as a first step.

tianshilei1992 added inline comments.Jul 25 2021, 7:31 AM
openmp/libomptarget/src/private.h
117

Is it necessary to implement it in libomp? interop is for target.

jdoerfert added inline comments.Jul 25 2021, 9:13 AM
openmp/libomptarget/src/private.h
117

eventually it is. if you don't add -fopenmp-targets you would otherwise get link errors, which sounds pretty bad.

tianshilei1992 added inline comments.Jul 25 2021, 9:52 AM
openmp/libomptarget/src/private.h
117

Well, we can implement a wrapper in libomp, just like omp_get_num_devices.

// Get number of NON-HOST devices.
// libomptarget, if loaded, provides this function in api.cpp.
int FTN_STDCALL KMP_EXPAND_NAME(FTN_GET_NUM_DEVICES)(void)
    KMP_WEAK_ATTRIBUTE_EXTERNAL;
int FTN_STDCALL KMP_EXPAND_NAME(FTN_GET_NUM_DEVICES)(void) {
#if KMP_MIC || KMP_OS_DARWIN || defined(KMP_STUB)
  return 0;
#else
  int (*fptr)();
  if ((*(void **)(&fptr) = KMP_DLSYM("__tgt_get_num_devices"))) {
    return (*fptr)();
  } else if ((*(void **)(&fptr) = KMP_DLSYM_NEXT("omp_get_num_devices"))) {
    return (*fptr)();
  } else if ((*(void **)(&fptr) = KMP_DLSYM("_Offload_number_of_devices"))) {
    return (*fptr)();
  } else { // liboffload & libomptarget don't exist
    return 0;
  }
#endif // KMP_MIC || KMP_OS_DARWIN || KMP_OS_WINDOWS || defined(KMP_STUB)
}
sriharikrishna marked 17 inline comments as done.Jul 26 2021, 9:11 PM
sriharikrishna added inline comments.
openmp/libomptarget/include/interop.h
20

Okay. Any suggestions on how to overcome this?

110

Okay. While linking, if we use the order -lomptarget -lomp then we get the values from libomptarget. If we use the order -lomp -lomptarget we get the values from libomp. This seems fragile.

openmp/libomptarget/include/omptarget.h
21

Undid it.

openmp/libomptarget/src/interop.cpp
178

It is not clear how to fix this. In the user code's interop directive, the interop variable must be of type 'omp_interop_t'. So getting

sriharikrishna marked 3 inline comments as done.

Runtime for Interop directive

sriharikrishna added inline comments.Jul 26 2021, 9:13 PM
openmp/libomptarget/src/interop.cpp
178

Incorrect comment above. This is fixed.

@AndreyChurbanov @tianshilei1992 Any concerns with merging this today if the two things below are addressed?

openmp/runtime/src/dllexports
553

Those values are taken by now, you need new values that are not taken yet.

openmp/runtime/src/kmp_ftn_entry.h
1546

We need the KMP_DLSYM_NEXT lookup in these, probably sufficient to do only that lookup though.

There are still many open comments.

sriharikrishna marked 8 inline comments as done.

Runtime for Interop directiv

I have fixed the latest comments.

Runtime for Interop directive

Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJul 27 2021, 7:38 PM

Runtime for Interop directive

Runtime for Interop directive

openmp/libomptarget/src/interop.cpp
14

I mean, this function doesn't have to start with __tgt because it is internal. Functions starting with __tgt are usually interface functions. From my perspective, it is better to name it as getPropertyErrorType, a.k.a. to use LLVM code style.

64

Same here

openmp/runtime/src/kmp_ftn_entry.h
1501

ditto

1515

ditto

1527

ditto

1539

ditto

1550

same as below

1561

same as below

1570–1574

Runtime for Interop directive

sriharikrishna marked 6 inline comments as done.Jul 28 2021, 1:47 PM
sriharikrishna marked 3 inline comments as done.
tianshilei1992 added inline comments.Jul 28 2021, 2:43 PM
openmp/libomptarget/plugins/cuda/src/rtl.cpp
1150–1156
openmp/libomptarget/src/interop.cpp
14

Looks better. Can you check https://llvm.org/docs/CodingStandards.html for the code style?

43

Does it make sense to use enum here?

openmp/libomptarget/src/interop.cpp
194–196

Need to check if device is available for all 3 __tgt_interop_init/use/destroy

198–201

Interop object does not wait for any task from libomp.
Init just initializes the interop object.
The initialization of interop object should be done by the plugin as only the plugin knows what properties are supported

239–242

Need to flush the queue if interop object was created with targetsync

260–263

You don't wait for the omp tasks. Need to flush the queue associated with the interop through the plugin

openmp/runtime/src/kmp_ftn_entry.h
1449–1494

Why do you have all this in openmp/runtime. Openmp should call libomptarget to get interop properties. if libomptarget is not loaded it should return 0

cchen added a subscriber: cchen.Aug 16 2021, 10:18 AM

We need a runtime test.

Synchronizing the asyn info object and signal out dependences will happen in the next revision.
So will minor edits as requested.

I commented below on some things.

openmp/libomptarget/src/interop.cpp
14

It's unclear given the mixed nature. Let's go with this and figure out if we need to rename vars later or not.

198–201

Interop object does not wait for any task from libomp.

I don't know why you think we would not wait for libomp tasks. If we have dependences we need to wait for them.

The initialization of interop object should be done by the plugin as only the plugin knows what properties are supported.

It is, below. This is the generic part that then redirects to the plugin.

260–263

Waiting for omp task is necessary and the above should do it. Flushing and signaling out dependences will be added in the next revision.

openmp/runtime/src/kmp_ftn_entry.h
1449–1494

That is exactly what this code does, no? Look for libomptarget and redirect to that one, otherwise return 0.
Unsure I see your point. FWIW, this is copied from other functions we duplicate in libomp but that actually are part of libomptarget.

openmp/libomptarget/src/interop.cpp
198–201

Libomp would have not invoked the task which calls this routine if there are dependences. They must be executed before the task containing the interop creation is scheduled.

The interop_type should be passed to plugin and let it initialize the common for all interop-types and then add based on the interop_type

jdoerfert added inline comments.Fri, Sep 10, 11:25 AM
openmp/libomptarget/src/interop.cpp
198–201

Libomp would have not invoked the task which calls this routine if there are dependences. They must be executed before the task containing the interop creation is scheduled.

To me it seems you are assuming that we create a task in which this routine is called. We do not, as far as I can tell. See D105876.

The interop_type should be passed to plugin and let it initialize the common for all interop-types and then add based on the interop_type

So what you are trying to say is that init_device_info should take the interop_type too? That makes sense to me. But as discussed in other reviews recently, we should not extend the API for "future use cases" but extend it as use cases become relevant. For now it seems we can simply set up the tgt_device_info part of the omp_interop_val_t without knowing the interop_type.

openmp/libomptarget/src/interop.cpp
198–201

Then need to change this code since the interop_type can be both target_sync and target which will not be handled correctly. target_sync and target have common initialization + additional property base don the interop_type requested

jdoerfert added inline comments.Fri, Sep 10, 2:25 PM
openmp/libomptarget/src/interop.cpp
198–201

Then need to change this code since the interop_type can be both target_sync and target which will not be handled correctly. target_sync and target have common initialization + additional property base don the interop_type requested

Could you please elaborate what needs to be changed exactly. What information is currently not available in the setup as is? What properties would be different?

openmp/libomptarget/src/interop.cpp
198–201

Should be something like this

NEED to add _kmp_interop_type_target to represent interop target
interop_ptr->device_info would initialize the following: device handle, device_context, platform.
if (interop_type == kmp_interop_type_target) {
if (!Device.RTL || !Device.RTL->init_device_info ||

  Device.RTL->init_device_info(device_id, &(interop_ptr)->device_info,
                               &(interop_ptr)->err_str)) {
delete interop_ptr;
interop_ptr = omp_interop_none;

}
// Add target sync if request.
if (interop_type == kmp_interop_type_tasksync) {

  if (!Device.RTL || !Device.RTL->init_async_info ||
      Device.RTL->init_async_info(device_id, &(interop_ptr)->async_info)) {
    delete interop_ptr;
    interop_ptr = omp_interop_none;
}