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

Samuel Antao via Phabricator via Openmp-commits openmp-commits at lists.llvm.org
Thu Jan 5 06:18:45 PST 2017


sfantao added a comment.

Geroge,

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



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


================
Comment at: libomptarget/CMakeLists.txt:96-97
+  
+  # Install libomptarget under the lib destination folder.
+  install(TARGETS omptarget LIBRARY DESTINATION "lib")
+  
----------------
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`.


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


================
Comment at: libomptarget/README.txt:62
+  - clang (development branch at http://clang.llvm.org - several features still 
+    under development)
+
----------------
This is only true with the change in `https://reviews.llvm.org/D28298` we need to make sure that is committed first.


================
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()
----------------
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.


https://reviews.llvm.org/D14031





More information about the Openmp-commits mailing list