[OpenMP] Initial implementation of OpenMP offloading library - libomptarget.
ClosedPublic

Authored by grokos on Oct 23 2015, 5:00 PM.

Details

Summary

This patches proposes an initial implementation of the libomptarget based on what we have working today in https://github.com/clang-omp.

I tried to follow what has been done in openmp/runtime in terms of the cmake build system.

The library sources are placed under libomptarget and it consists of three components:

  • device agnostic library (the interface that clang uses)
  • device plugins - right now we create a plug-in for CUDA-enabled devices, and a generic 64-bit plugin that works on powerpcBE/LE and x86_64 (mostly for testing purposes)
  • device runtime library for CUDA enabled devices.

The interface of the plugins and target agnostic library are documented in http://goo.gl/L1rnKJ.

I included the logic for testing based on llvm-lit and FileCheck. Currently there are only two regression tests that basically check that offloading failed. This is because the target side codegen (http://reviews.llvm.org/D12614) and driver support (http://reviews.llvm.org/D9888) are still under review in clang. Once that functionality is in clang I plan to include more tests to exercise the library.

In this patch, I tried to organize things based on what makes sense to me and also based on previous discussions with other OpenMP project contributors. I am happy to organize things in a different way if suggested to do so - the goal here is also open the discussion on how this library should be contributed to the the project.

Let me now any suggestions/comments you may have,

Thanks!
Samuel

Diff Detail

Repository
rL LLVM
There are a very large number of changes, so older changes are hidden. Show Older Changes
This revision is now accepted and ready to land.Dec 2 2016, 4:26 AM
Hahnfeld requested changes to this revision.Dec 3 2016, 9:14 AM
Hahnfeld added a reviewer: Hahnfeld.

Full review and comments all over

libomptarget/Build_With_CMake.txt
102–115 ↗(On Diff #79982)

This should probably go under NVPTX device RTL specific

libomptarget/CMakeLists.txt
96–97 ↗(On Diff #79982)

There is LLVM_LIBDIR_SUFFIX which should probably be respected (compare to LIBOMP_LIBDIR_SUFFIX)

106–108 ↗(On Diff #79982)

These are not part of this patch and should therefore not be added here

libomptarget/README.txt
60 ↗(On Diff #79982)

Is that true? I remember there were some changes to the section names in the fatbinary?

libomptarget/src/omptarget.cpp
122–130 ↗(On Diff #79982)

Could this reuse the assignment operator? Or otherwise do this in the initializer list

255–257 ↗(On Diff #79982)

Please move this up, directly below struct DeviceTy

283 ↗(On Diff #79982)

This is not in the standard, should this really start with OMP_?

284–285 ↗(On Diff #79982)

can be collapsed

421–422 ↗(On Diff #79982)

can be collapsed

494 ↗(On Diff #79982)

Do we want to check device_is_ready here?

508–511 ↗(On Diff #79982)

Should we have a list of all allocated addresses above?

513 ↗(On Diff #79982)

Do we want to check device_is_ready here?

532–533 ↗(On Diff #79982)

can be collapsed

538–539 ↗(On Diff #79982)

likewise

910 ↗(On Diff #79982)

bool ForceDelete?

1106 ↗(On Diff #79982)

if no RTL was found?

1112–1136 ↗(On Diff #79982)

Can't this be done immediately after the corresponding RTL was found the same way we register the image into the translation table? Maybe also refactor the code into another static function?

1114 ↗(On Diff #79982)

I think this should either be Devices[FoundRTL->Idx + i] or *FoundRTL->Devices[i]?

1177 ↗(On Diff #79982)

I think this should either be Devices[FoundRTL->Idx + i] or *FoundRTL->Devices[i]?

1193–1205 ↗(On Diff #79982)

I think this is only needed once per desc and not per img, isn't it?

1354–1355 ↗(On Diff #79982)

The Device should only be fetched after the device_id has been proved valid

1357–1377 ↗(On Diff #79982)

This looks the same as device_is_ready, can this be reused?

1383–1384 ↗(On Diff #79982)

can be collapsed

1393–1395 ↗(On Diff #79982)

Backwards-compatible on the first check-in? That sounds weird...

2116 ↗(On Diff #79982)

break afterwards?

2131 ↗(On Diff #79982)

likewise?

libomptarget/src/omptarget.h
86 ↗(On Diff #79982)

Maybe rename this to NumDeviceImages?

88–89 ↗(On Diff #79982)

Can those be renamed to have Host in the same so that I'm not that much confused? :D

166 ↗(On Diff #79982)

Only as data_end, not as data_update?

libomptarget/test/CMakeLists.txt
70–76 ↗(On Diff #79982)
  1. This will abort CMake which probably isn't a good idea.
  2. MSVC is possible for standalone builds?
This revision now requires changes to proceed.Dec 3 2016, 9:14 AM
sfantao added a comment.EditedJan 5 2017, 6:22 AM

George,

Thanks for the patch. Here are few comments following Jonas review.

libomptarget/CMakeLists.txt
38 ↗(On Diff #79982)

Following Jonas suggestion, we can select the install suffix here like libomp does. Something like:

if(${LIBOMPTARGET_STANDALONE_BUILD})
  set(LIBOMPTARGET_ENABLE_WERROR FALSE CACHE BOOL
    "Enable -Werror flags to turn warnings into errors for supporting compilers.")
  # CMAKE_BUILD_TYPE was not defined, set default to Release
  if(NOT CMAKE_BUILD_TYPE)
    set(CMAKE_BUILD_TYPE Release)
  endif()
  set(LIBOMPTARGET_LIBDIR_SUFFIX "" CACHE STRING
    "suffix of lib installation directory e.g., 64 => lib64")
else()
  set(LIBOMPTARGET_ENABLE_WERROR ${LLVM_ENABLE_WERROR})
  # If building in tree, we honor the same install suffix LLVM uses.
  set(LIBOMPTARGET_LIBDIR_SUFFIX ${LLVM_LIBDIR_SUFFIX})
endif()

Note that we have LIBOMP_ENABLE_WERROR, that should be LIBOMPTARGET_ENABLE_WERROR.

96–97 ↗(On Diff #79982)

Right, following what we selected before we can use lib${LIBOMPTARGET_LIBDIR_SUFFIX} instead of just lib.

libomptarget/README.txt
60 ↗(On Diff #79982)

Good catch. The library is compatible with what is https://github.com/clang-ykt, we should probably use that instead. Unlike http://clang-omp.github.io/, it contains the latest Sema/CodeGen reimplementation that has been happening in the trunk.

Also, as Jonas mentioned, some section naming is different and the offload entries descriptors contain more information.

62 ↗(On Diff #79982)

This is only true with the change in https://reviews.llvm.org/D28298 we need to make sure that is committed first.

libomptarget/test/CMakeLists.txt
70–76 ↗(On Diff #79982)
  1. Ok, we should probably replace this by a warning instead of using an error.
  1. It should be possible. The reason of the error/warning is that it is untested. We try not to do anything that precludes MSVC builds - we also try to pave the way for that to happen by preparing some MSVC options as currently used by other components of LLVM and libomp. However, the goal of our contribution is to add support for linux machines which are the machines we and our users have access to. This is true for this library but is also true for the compiler support - it will need tuning/features to fully work on Windows. We expect someone with interest in having support for Windows to complete the work by contributing the features and testing infrastructure for that.
grokos updated this revision to Diff 84399.Jan 13 2017, 3:14 PM
grokos marked 31 inline comments as done.

Addressed more comments, awaiting new review.

libomptarget/Build_With_CMake.txt
102–115 ↗(On Diff #79982)

We only have one instruction file for the whole library. After all, cmake is invoked once from libomptarget's root directory, all -D definitions are passed to the "root" cmake.

I am happy to revise this scheme if you think it makes more sense to split the instructions, although I strongly prefer the current implementation.

libomptarget/CMakeLists.txt
96–97 ↗(On Diff #79982)

OK, I incorporated the changes in the new diff. Thanks for the suggestion!

106–108 ↗(On Diff #79982)

OK, I'll make them part of the corresponding patches for plugins and RTLs.

libomptarget/README.txt
60 ↗(On Diff #79982)

I updated the documentation in the new diff.

libomptarget/src/omptarget.cpp
122–130 ↗(On Diff #79982)

It's not efficient to reuse the assignment operator. I've done this in the initializer list instead.

255–257 ↗(On Diff #79982)

Done in new diff.

283 ↗(On Diff #79982)

Our buildbot already uses this env var name, but I am happy to change it to whatever name you propose. However, I don't see any problem with OMP_, especially if there is any chance that future revisions of the standard introduce such an env var.

421–422 ↗(On Diff #79982)

Done.

494 ↗(On Diff #79982)

Right, thanks for pointing it out!

508–511 ↗(On Diff #79982)

No, omp_target_is_present checks whether some host address has corresponding storage on the device, it's got nothing to do with memory allocated via omp_target_alloc. So, in order to check whether a host address has been mapped we just use the device's HostDataToTargetMap.

513 ↗(On Diff #79982)

It's not necessary. omp_target_is_present only accesses the device's HostDataToTargetMap to determine whether an address has been mapped. If the device is not initialized, HostDataToTargetMap has zero contents and omp_target_is_present correctly returns false. The function does not make any call to RTL functions (like data_alloc or data_free), so even if the device has not been initialized there is no problem.

532–533 ↗(On Diff #79982)

Done.

538–539 ↗(On Diff #79982)

Done.

910 ↗(On Diff #79982)

I've changed most instances where long served as a bool, i.e. variables IsNew, IsLast, Forcedelete, IsImplicit etc.

I also realized that long was used for array sizes, whereas the correct datatype should be int64_t (that's the datatype used by clang to communicate with the interface functions). I fixed that too.

1106 ↗(On Diff #79982)

Thanks for spotting this.

1112–1136 ↗(On Diff #79982)

Done. I refactored the code into a new static function which is called right after registering the image into the translation table.

1114 ↗(On Diff #79982)

Correct, fixed in new diff.

1177 ↗(On Diff #79982)

Fixed.

1193–1205 ↗(On Diff #79982)

Correct.

1354–1355 ↗(On Diff #79982)

Fixed.

1357–1377 ↗(On Diff #79982)

Correct, I've reused the former function in the new diff.

1383–1384 ↗(On Diff #79982)

Done.

1393–1395 ↗(On Diff #79982)

Backwards compatible with internal, unreleased versions of clang. Oh well....

2116 ↗(On Diff #79982)

Thanks for spotting this, obviously if offload fails at some point there is no need to continue with the loop.

2131 ↗(On Diff #79982)

Done.

libomptarget/src/omptarget.h
86 ↗(On Diff #79982)

You're right, some variable names caused confusion, I've changed them :)

88–89 ↗(On Diff #79982)

Done.

166 ↗(On Diff #79982)

Right, it's only data_end.

libomptarget/test/CMakeLists.txt
70–76 ↗(On Diff #79982)

OK, I replaced the fatal error with a warning.

Only a minor comment for the code. And I think you may have missed some changes for the uploaded diff?

libomptarget/Build_With_CMake.txt
102–115 ↗(On Diff #79982)

I just meant that these options should be moved a few lines below so that they are listed under the secion NVPTX device RTL specific in this document

libomptarget/CMakeLists.txt
38 ↗(On Diff #79982)

LIBOMP_ENABLE_WERROR is not yet fixed

96–97 ↗(On Diff #79982)

I don't see the suffix here, maybe you forgot to add some changes to this file?

libomptarget/README.txt
60 ↗(On Diff #79982)

I don't see that neither

libomptarget/src/omptarget.cpp
1375–1376 ↗(On Diff #84399)

I think this has not yet been collapsed...

283 ↗(On Diff #79982)

Ok, let's keep it like that. But if that ever happens, we can't ensure any backwards compatibility...

513 ↗(On Diff #79982)

Alright. I think we should then check for device_num < Devices.size() because operator [] will not perform any bounds check. Just in case the user supplies an invalid parameter?

1393–1395 ↗(On Diff #79982)

Hmm, when is that planned for removal then?

libomptarget/test/CMakeLists.txt
70–76 ↗(On Diff #79982)

I still see libomptarget_error_say here...

grokos marked 7 inline comments as done.Jan 16 2017, 5:12 PM

Jonas, thanks for the comments once again. It seems I had forgotten to add the CMakeLists files to the git commit. Here is the latest revision of the patch.

libomptarget/Build_With_CMake.txt
102–115 ↗(On Diff #79982)

OK, I moved them.

libomptarget/src/omptarget.cpp
1375–1376 ↗(On Diff #84399)

Done.

513 ↗(On Diff #79982)

Good catch! I added a check.

1393–1395 ↗(On Diff #79982)

It will be removed once clang starts using internally the new map types. As far as I know, we haven't started any work on that yet. The async interface is our next priority, upgrading clang to use the new map types will come afterwards.

grokos updated this revision to Diff 84614.Jan 16 2017, 5:15 PM
grokos marked 3 inline comments as done.

Took care of the last few comments.

Hahnfeld accepted this revision.Jan 17 2017, 12:09 AM

LGTM, thanks for your patience!

This revision is now accepted and ready to land.Jan 17 2017, 12:09 AM
This revision was automatically updated to reflect the committed changes.