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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Looks like openmp/runtime/src/dllexports is edited by mistake. Added symbols are defined nowhere that causes build failure on Windows.
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 | ||
79 | I think these series of changes regarding SPMD_GENERIC has nothing to do with interop. Please make a separate patch for them. | |
1238–1239 | ||
openmp/libomptarget/src/CMakeLists.txt | ||
21 | order please | |
openmp/libomptarget/src/exports | ||
51 | Like @RaviNarayanaswamy said, better to rename them to __tgt_xxx. | |
openmp/libomptarget/src/interop.cpp | ||
14 | There are some conventions in current libomptarget.
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 | ||
113 | 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. In libomptarget already have the definitions and exports. | |
161 | Remove the asserts. | |
openmp/libomptarget/plugins/cuda/src/rtl.cpp | ||
79 | 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. | |
openmp/libomptarget/src/private.h | ||
113 | It is in libomp, this is a forward decl. All good here. | |
openmp/runtime/src/dllexports | ||
587 | 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. |
openmp/libomptarget/src/interop.cpp | ||
---|---|---|
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 |
openmp/libomptarget/src/private.h | ||
---|---|---|
113 | Is it necessary to implement it in libomp? interop is for target. |
openmp/libomptarget/src/private.h | ||
---|---|---|
113 | eventually it is. if you don't add -fopenmp-targets you would otherwise get link errors, which sounds pretty bad. |
openmp/libomptarget/src/private.h | ||
---|---|---|
113 | 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) } |
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 |
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 | ||
---|---|---|
556 | 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. |
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 |
openmp/libomptarget/plugins/cuda/src/rtl.cpp | ||
---|---|---|
1251–1257 | ||
openmp/libomptarget/src/interop.cpp | ||
14 | Looks better. Can you check https://llvm.org/docs/CodingStandards.html for the code style? | |
44 | Does it make sense to use enum here? |
openmp/libomptarget/src/interop.cpp | ||
---|---|---|
195–197 | Need to check if device is available for all 3 __tgt_interop_init/use/destroy | |
199–202 | Interop object does not wait for any task from libomp. | |
240–243 | Need to flush the queue if interop object was created with targetsync | |
261–264 | 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 |
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. | |
199–202 |
I don't know why you think we would not wait for libomp tasks. If we have dependences we need to wait for them.
It is, below. This is the generic part that then redirects to the plugin. | |
261–264 | 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. |
openmp/libomptarget/src/interop.cpp | ||
---|---|---|
199–202 | 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 |
openmp/libomptarget/src/interop.cpp | ||
---|---|---|
199–202 |
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.
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 | ||
---|---|---|
199–202 | 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 |
openmp/libomptarget/src/interop.cpp | ||
---|---|---|
199–202 |
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 | ||
---|---|---|
199–202 | Should be something like this NEED to add _kmp_interop_type_target to represent interop target Device.RTL->init_device_info(device_id, &(interop_ptr)->device_info, &(interop_ptr)->err_str)) { delete interop_ptr; interop_ptr = omp_interop_none; } 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; } |
openmp/libomptarget/src/interop.cpp | ||
---|---|---|
199–202 | It looks like the code you have written for kmp_interop_type_target already exists in the else part. So I am still puzzled about this. |
openmp/runtime/src/dllexports | ||
---|---|---|
556 | This broke building for Windows: generate-def.pl: (x) Error parsing file "llvm-project/openmp/runtime/src/dllexports" line 556: generate-def.pl: (x) omp_get_interop_int 2514 generate-def.pl: (x) Ordinal of user-callable entry must be < 1000 |
openmp/runtime/src/dllexports | ||
---|---|---|
556 | ... and even with those ordinals changed to something in the right range, the build later fails with linker errors: ld.lld: error: <root>: undefined symbol: OMP_GET_INTEROP_INT ld.lld: error: <root>: undefined symbol: OMP_GET_INTEROP_PTR ld.lld: error: <root>: undefined symbol: OMP_GET_INTEROP_STR So please fix the Windows build of OpenMP, before the 14.x branch is made early next week. |
openmp/runtime/src/dllexports | ||
---|---|---|
556 | As the warning says these should be < 1000, because lowercase are automatically replicated in uppercase adding +1000 to the ordinal and still be < 2000 (for some particular Fortran usages). Actually these interfaces described as "C/C++"-only in the specification, so there is no need to provide Fortran-specific versions, though it should not harm. Probably making them C-only does not worth efforts for now. |
We need the llvm license header, copied from another file.