[Openmp-commits] [PATCH] D14031: [OpenMP] Initial implementation of OpenMP offloading library - libomptarget.

Sat Jan 14 08:43:06 PST 2017

Hahnfeld added a comment.

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

================
Comment at: libomptarget/Build_With_CMake.txt:102-115
+-DLIBOMPTARGET_NVPTX_ENABLE_BCLIB=false|true
+Enable CUDA LLVM bitcode offloading device RTL. This is used for
+link time optimization of the omp runtime and application code.
+
+-DLIBOMPTARGET_NVPTX_CUDA_COMPILER=<CUDA compiler name>
+Location of a CUDA compiler capable of emitting LLVM bitcode.
+Currently only the Clang compiler is supported. This is only used
----------------
grokos wrote:
> Hahnfeld wrote:
> > This should probably go under `NVPTX device RTL specific`
> 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.
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

================
Comment at: libomptarget/CMakeLists.txt:38
+  set(LIBOMP_ENABLE_WERROR ${LLVM_ENABLE_WERROR})
+endif()
+
----------------
sfantao wrote:
> 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. 
`LIBOMP_ENABLE_WERROR` is not yet fixed

================
Comment at: libomptarget/CMakeLists.txt:96-97
+  
+  # Install libomptarget under the lib destination folder.
+  install(TARGETS omptarget LIBRARY DESTINATION "lib")
+  
----------------
grokos wrote:
> sfantao wrote:
> > Hahnfeld wrote:
> > > There is `LLVM_LIBDIR_SUFFIX` which should probably be respected (compare to `LIBOMP_LIBDIR_SUFFIX`)
> > Right, following what we selected before we can use `lib${LIBOMPTARGET_LIBDIR_SUFFIX}` instead of just `lib`.
> OK, I incorporated the changes in the new diff. Thanks for the suggestion!
I don't see the suffix here, maybe you forgot to add some changes to this file?

================
Comment at: libomptarget/README.txt:60
+this RTL: 
+  - clang (from the OpenMP development branch at http://clang-omp.github.io/ )
+  - clang (development branch at http://clang.llvm.org - several features still 
----------------
grokos wrote:
> sfantao wrote:
> > Hahnfeld wrote:
> > > Is that true? I remember there were some changes to the section names in the fatbinary?
> > 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.
> I updated the documentation in the new diff.
I don't see that neither

================
Comment at: libomptarget/src/omptarget.cpp:283
+  // Parse environment variable OMP_TARGET_OFFLOAD (if set)
+  char *envStr = getenv("OMP_TARGET_OFFLOAD");
+  if (envStr) {
----------------
grokos wrote:
> Hahnfeld wrote:
> > This is not in the standard, should this really start with `OMP_`?
> 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.
Ok, let's keep it like that. But if that ever happens, we can't ensure any backwards compatibility...

================
Comment at: libomptarget/src/omptarget.cpp:513
+
+  DeviceTy& Device = Devices[device_num];
+  long IsLast; // not used
----------------
grokos wrote:
> Hahnfeld wrote:
> > Do we want to check `device_is_ready` here?
> 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.
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?

================
Comment at: libomptarget/src/omptarget.cpp:1393-1395
+// Following datatypes and functions (tgt_oldmap_type, combined_entry_t,
+// translate_map, cleanup_map) will be removed once the compiler starts using
+// the new map types.
----------------
grokos wrote:
> Hahnfeld wrote:
> > Backwards-compatible on the first check-in? That sounds weird...
> Backwards compatible with internal, unreleased versions of clang. Oh well....
Hmm, when is that planned for removal then?

================
Comment at: libomptarget/src/omptarget.cpp:1375-1376
+  Device.PendingGlobalsMtx.unlock();
+  if (hasPendingGlobals) {
+    if (InitLibrary(Device) != OFFLOAD_SUCCESS) {
+      DP("Failed to init globals on device %d\n", device_id);
----------------
I think this has not yet been collapsed...

================
Comment at: libomptarget/test/CMakeLists.txt:70-76
+  if(NOT MSVC)
+    set(LIBOMPTARGET_TEST_C_COMPILER ${LLVM_RUNTIME_OUTPUT_INTDIR}/clang)
+    set(LIBOMPTARGET_TEST_CXX_COMPILER ${LLVM_RUNTIME_OUTPUT_INTDIR}/clang++)
+    set(LIBOMPTARGET_FILECHECK ${LLVM_RUNTIME_OUTPUT_INTDIR}/FileCheck)
+  else()
+    libomptarget_error_say("Not prepared to run tests on Windows systems.")
+  endif()
----------------
grokos wrote:
> sfantao wrote:
> > Hahnfeld wrote:
> > > 1. This will abort CMake which probably isn't a good idea.
> > > 2. `MSVC` is possible for standalone builds?
> > 1. Ok, we should probably replace this by a warning instead of using an error.
> > 
> > 2. 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.
> OK, I replaced the fatal error with a warning.
I still see `libomptarget_error_say` here...

https://reviews.llvm.org/D14031