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

[George Rokos via Phabricator via Openmp-commits]

grokos commandeered this revision.
grokos added a reviewer: sfantao.
grokos added inline comments.
Herald added a subscriber: mgorny.


================
Comment at: libomptarget/cmake/Modules/LibomptargetGetDependencies.cmake:63-78
+# Looking for libffi...
+################################################################################
+find_package(PkgConfig)
+
+pkg_check_modules(LIBOMPTARGET_SEARCH_LIBFFI QUIET libffi)
+
+find_path (
----------------
sfantao wrote:
> Hahnfeld wrote:
> > After looking at this quite some time and wondering why it didn't work: It's CMake, they use curly braces. With this small change everything builds correctly out-of-the-box, thanks!
> Oh, right, sorry, my bad.... The new diff fixes it.
So I'm marking this issue as "done".


================
Comment at: libomptarget/src/omptarget.cpp:348
+        DP("Deleting tgt data 0x%016llx of size %lld\n",
+           (long long)HT.TgtPtrBegin, (long long)Size);
+        RTL->data_delete(RTLDeviceID, (void *)HT.TgtPtrBegin);
----------------
cbergstrom wrote:
> Instead of casting to long long and friends - what about making the type  uintptr_t ?
Done in the new diff.


================
Comment at: libomptarget/src/omptarget.cpp:723-726
+  if (!Device.IsInit) {
+    DP("uninit device: ignore");
+    return;
+  }
----------------
sfantao wrote:
> Hahnfeld wrote:
> > sergos wrote:
> > > Hahnfeld wrote:
> > > > I think this assumption may be wrong: The standard defines that each device has an initial data environment (section 1.4.2).
> > > > 
> > > > Therefore a `#pragma omp target update` may be a first (and valid) statement which means that it has to be init here as well. Currently the library will in such a case return at this point and ignore the data transfer.
> > > not quite relevant. the initial data environment should be initialized at the point of __tgt_register_lib() call that should prepend any data/control transfer to/from target. that's why we can say the target image was not initialized yet.
> > I think the plan was to lazily initialize the device once it is really needed.
> > 
> > I support this because there may be more libs registered than actually used. So it would be wasted to initialize all of their devices and transfer data to them.
> I agree with Jonas, we should allow the device to be initialized in target update so that we only copy the data for that device and not other devices that may not be used at all by the application. I'll fix this.
Done in new diff.


================
Comment at: libomptarget/src/omptarget.cpp:929
+  // Move data to device.
+  target_data_begin(Device, arg_num, args_base, args, arg_sizes, arg_types);
+
----------------
hyviquel wrote:
> Why the offloading is continuing if the mapping of data already failed... 
> `target_data_begin` could return `OFFLOAD_FAIL` or `OFFLOAD_SUCCESS` to allow the offloading to stop earlier
I fixed that behavior in the new diff. If target_data_begin fails, the offloading stops.


================
Comment at: libomptarget/src/omptarget.cpp:980
+  // Move data from device.
+  target_data_end(Device, arg_num, args_base, args, arg_sizes, arg_types);
+  return OFFLOAD_SUCCESS;
----------------
hyviquel wrote:
> you return OFFLOAD_SUCCESS even if something went wrong in `target_data_end`
Fixed.


================
Comment at: libomptarget/test/CMakeLists.txt:28-34
+  find_program(LIT_EXECUTABLE NAMES llvm-lit ${LIBOMPTARGET_LLVM_LIT_EXECUTABLE})
+  if(NOT LIT_EXECUTABLE)
+    libomptarget_say("Cannot find llvm-lit.")
+    libomptarget_say("Please put llvm-lit in your PATH or set LIBOMPTARGET_LLVM_LIT_EXECUTABLE to its full path")
+    libomptarget_warning_say("The check-libomptarget target will not be available!")
+    return()
+  endif()
----------------
Hahnfeld wrote:
> Maybe we could later on think about sharing some testing setup between `libomp` and `libomptarget`...
Sure, let's keep this on our list for future implementation.


================
Comment at: libomptarget/test/CMakeLists.txt:64-67
+  set(LIBOMPTARGET_OPENMP_HEADER_FOLDER "" CACHE STRING
+    "Path to folder containing omp.h")
+  set(LIBOMPTARGET_OPENMP_HOST_RTL_FOLDER "" CACHE STRING
+    "Path to folder containing libomp.h")
----------------
Hahnfeld wrote:
> I think this can safely assume the relative location of `../runtime/src`
Actually, it's `../../runtime/src`. I've hardcoded it as:

`${CMAKE_CURRENT_BINARY_DIR}/../../runtime/src`


================
Comment at: libomptarget/test/offloading/offloading_success.c:9
+int main(void) {
+  int isHost = 0;
+
----------------
Hahnfeld wrote:
> Maybe make this `isHost = 1`. clang 3.7.1 seems to completely ignore the target code, so `omp_is_initial_device` is never called...
With the current version of clang this should work. Let me know if I'm mistaken and I'll push another patch.


================
Comment at: libomptarget/test/offloading/offloading_success.cpp:9
+int main(void) {
+  int isHost = 0;
+
----------------
Hahnfeld wrote:
> likewise
Same here...


https://reviews.llvm.org/D14031