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
103–116

This should probably go under NVPTX device RTL specific

libomptarget/CMakeLists.txt
97–98

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

107–109

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

libomptarget/README.txt
61

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

libomptarget/src/omptarget.cpp
123–131

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

256–258

Please move this up, directly below struct DeviceTy

284

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

285–286

can be collapsed

422–423

can be collapsed

495

Do we want to check device_is_ready here?

509–512

Should we have a list of all allocated addresses above?

514

Do we want to check device_is_ready here?

533–534

can be collapsed

539–540

likewise

911

bool ForceDelete?

1107

if no RTL was found?

1113–1137

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?

1115

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

1178

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

1194–1206

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

1355–1356

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

1358–1378

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

1384–1385

can be collapsed

1394–1396

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

2117

break afterwards?

2132

likewise?

libomptarget/src/omptarget.h
87

Maybe rename this to NumDeviceImages?

89–90

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

167

Only as data_end, not as data_update?

libomptarget/test/CMakeLists.txt
71–77
  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
39

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.

97–98

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

libomptarget/README.txt
61

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.

63

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
71–77
  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
103–116

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
97–98

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

107–109

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

libomptarget/README.txt
61

I updated the documentation in the new diff.

libomptarget/src/omptarget.cpp
123–131

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

256–258

Done in new diff.

284

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.

422–423

Done.

495

Right, thanks for pointing it out!

509–512

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.

514

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.

533–534

Done.

539–540

Done.

911

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.

1107

Thanks for spotting this.

1113–1137

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

1115

Correct, fixed in new diff.

1178

Fixed.

1194–1206

Correct.

1355–1356

Fixed.

1358–1378

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

1384–1385

Done.

1394–1396

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

2117

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

2132

Done.

libomptarget/src/omptarget.h
87

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

89–90

Done.

167

Right, it's only data_end.

libomptarget/test/CMakeLists.txt
71–77

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
103–116

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
39

LIBOMP_ENABLE_WERROR is not yet fixed

97–98

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

libomptarget/README.txt
61

I don't see that neither

libomptarget/src/omptarget.cpp
284

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

514

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...

1394–1396

Hmm, when is that planned for removal then?

libomptarget/test/CMakeLists.txt
71–77

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
103–116

OK, I moved them.

libomptarget/src/omptarget.cpp
514

Good catch! I added a check.

1375–1376

Done.

1394–1396

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.