Page MenuHomePhabricator

[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

Event Timeline

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

This should probably go under NVPTX device RTL specific

libomptarget/CMakeLists.txt
96–97

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

106–108

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

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

255–257

Please move this up, directly below struct DeviceTy

283

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

284–285

can be collapsed

421–422

can be collapsed

494

Do we want to check device_is_ready here?

508–511

Should we have a list of all allocated addresses above?

513

Do we want to check device_is_ready here?

532–533

can be collapsed

538–539

likewise

910

bool ForceDelete?

1106

if no RTL was found?

1112–1136

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

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

1177

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

1193–1205

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

1354–1355

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

1357–1377

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

1383–1384

can be collapsed

1393–1395

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

2116

break afterwards?

2131

likewise?

libomptarget/src/omptarget.h
86

Maybe rename this to NumDeviceImages?

88–89

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

166

Only as data_end, not as data_update?

libomptarget/test/CMakeLists.txt
70–76
  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 edited edge metadata.EditedJan 5 2017, 6:22 AM

George,

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

libomptarget/CMakeLists.txt
38

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

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
  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 edited edge metadata.
grokos marked 31 inline comments as done.

Addressed more comments, awaiting new review.

libomptarget/Build_With_CMake.txt
102–115

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

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

106–108

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

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

255–257

Done in new diff.

283

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

Done.

494

Right, thanks for pointing it out!

508–511

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

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

Done.

538–539

Done.

910

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

Thanks for spotting this.

1112–1136

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

1114

Correct, fixed in new diff.

1177

Fixed.

1193–1205

Correct.

1354–1355

Fixed.

1357–1377

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

1383–1384

Done.

1393–1395

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

2116

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

2131

Done.

libomptarget/src/omptarget.h
86

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

88–89

Done.

166

Right, it's only data_end.

libomptarget/test/CMakeLists.txt
70–76

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

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

LIBOMP_ENABLE_WERROR is not yet fixed

96–97

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
283

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

513

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?

1375–1376

I think this has not yet been collapsed...

1393–1395

Hmm, when is that planned for removal then?

libomptarget/test/CMakeLists.txt
70–76

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

OK, I moved them.

libomptarget/src/omptarget.cpp
513

Good catch! I added a check.

1375–1376

Done.

1393–1395

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.