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

Jonas Hahnfeld via Phabricator via Openmp-commits openmp-commits at lists.llvm.org
Sat Dec 3 09:14:27 PST 2016


Hahnfeld requested changes to this revision.
Hahnfeld added a reviewer: Hahnfeld.
Hahnfeld added a comment.
This revision now requires changes to proceed.

Full review and comments all over



================
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
----------------
This should probably go under `NVPTX device RTL specific`


================
Comment at: libomptarget/CMakeLists.txt:96-97
+  
+  # Install libomptarget under the lib destination folder.
+  install(TARGETS omptarget LIBRARY DESTINATION "lib")
+  
----------------
There is `LLVM_LIBDIR_SUFFIX` which should probably be respected (compare to `LIBOMP_LIBDIR_SUFFIX`)


================
Comment at: libomptarget/CMakeLists.txt:106-108
+  # Build offloading plugins and device RTLs if they are available.
+  add_subdirectory(plugins)
+  add_subdirectory(deviceRTLs)
----------------
These are not part of this patch and should therefore not be added here


================
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 
----------------
Is that true? I remember there were some changes to the section names in the fatbinary?


================
Comment at: libomptarget/src/omptarget.cpp:122-130
+    DeviceID = d.DeviceID;
+    RTL = d.RTL;
+    RTLDeviceID = d.RTLDeviceID;
+    IsInit = d.IsInit;
+    HasPendingGlobals = d.HasPendingGlobals;
+    HostDataToTargetMap = d.HostDataToTargetMap;
+    PendingCtorsDtors = d.PendingCtorsDtors;
----------------
Could this reuse the assignment operator? Or otherwise do this in the initializer list


================
Comment at: libomptarget/src/omptarget.cpp:255-257
+/// Map between Device ID (i.e. openmp device id) and its DeviceTy.
+typedef std::vector<DeviceTy> DevicesTy;
+static DevicesTy Devices;
----------------
Please move this up, directly below `struct DeviceTy`


================
Comment at: libomptarget/src/omptarget.cpp:283
+  // Parse environment variable OMP_TARGET_OFFLOAD (if set)
+  char *envStr = getenv("OMP_TARGET_OFFLOAD");
+  if (envStr) {
----------------
This is not in the standard, should this really start with `OMP_`?


================
Comment at: libomptarget/src/omptarget.cpp:284-285
+  char *envStr = getenv("OMP_TARGET_OFFLOAD");
+  if (envStr) {
+    if (!strcmp(envStr, "DISABLED")) {
+      DP("Target offloading disabled by environment\n");
----------------
can be collapsed


================
Comment at: libomptarget/src/omptarget.cpp:421-422
+  // Init the device if not done before
+  if (!Device.IsInit) {
+    if (Device.initOnce() != OFFLOAD_SUCCESS) {
+      DP("Failed to init device %d\n", device_num);
----------------
can be collapsed


================
Comment at: libomptarget/src/omptarget.cpp:494
+
+  DeviceTy &Device = Devices[device_num];
+  Device.RTL->data_delete(Device.RTLDeviceID, (void *)device_ptr);
----------------
Do we want to check `device_is_ready` here?


================
Comment at: libomptarget/src/omptarget.cpp:508-511
+  if (device_num == omp_get_initial_device()) {
+    DP("Call to omp_target_is_present on host, returning true\n");
+    return true;
+  }
----------------
Should we have a list of all allocated addresses above?


================
Comment at: libomptarget/src/omptarget.cpp:513
+
+  DeviceTy& Device = Devices[device_num];
+  long IsLast; // not used
----------------
Do we want to check `device_is_ready` here?


================
Comment at: libomptarget/src/omptarget.cpp:532-533
+
+  if (src_device != omp_get_initial_device())
+    if (!device_is_ready(src_device)) {
+      DP("omp_target_memcpy returns OFFLOAD_FAIL\n");
----------------
can be collapsed


================
Comment at: libomptarget/src/omptarget.cpp:538-539
+
+  if (dst_device != omp_get_initial_device())
+    if (!device_is_ready(dst_device)) {
+      DP("omp_target_memcpy returns OFFLOAD_FAIL\n");
----------------
likewise


================
Comment at: libomptarget/src/omptarget.cpp:910
+
+int DeviceTy::deallocTgtPtr(void *HstPtrBegin, long Size, long ForceDelete) {
+  // Check if the pointer is contained in any sub-nodes.
----------------
`bool ForceDelete`?


================
Comment at: libomptarget/src/omptarget.cpp:1106
+
+    // if an RTL was found we are done - proceed to register the next image
+    if (!FoundRTL) {
----------------
`if no RTL was found`?


================
Comment at: libomptarget/src/omptarget.cpp:1112-1136
+    // Load ctors/dtors for static objects
+    for (int32_t i = 0; i < FoundRTL->NumberOfDevices; ++i) {
+      DeviceTy &Device = Devices[i];
+      Device.PendingGlobalsMtx.lock();
+      Device.HasPendingGlobals = true;
+      for (__tgt_offload_entry *entry = img->EntriesBegin;
+          entry != img->EntriesEnd; ++entry) {
----------------
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?


================
Comment at: libomptarget/src/omptarget.cpp:1114
+    for (int32_t i = 0; i < FoundRTL->NumberOfDevices; ++i) {
+      DeviceTy &Device = Devices[i];
+      Device.PendingGlobalsMtx.lock();
----------------
I think this should either be `Devices[FoundRTL->Idx + i]` or `*FoundRTL->Devices[i]`?


================
Comment at: libomptarget/src/omptarget.cpp:1177
+      for (int32_t i = 0; i < FoundRTL->NumberOfDevices; ++i) {
+        DeviceTy &Device = Devices[i];
+        Device.PendingGlobalsMtx.lock();
----------------
I think this should either be `Devices[FoundRTL->Idx + i]` or `*FoundRTL->Devices[i]`?


================
Comment at: libomptarget/src/omptarget.cpp:1193-1205
+      // Remove translation table for this image.
+      TrlTblMtx.lock();
+      auto tt = HostEntriesBeginToTransTable.find(desc->EntriesBegin);
+      if (tt != HostEntriesBeginToTransTable.end()) {
+        HostEntriesBeginToTransTable.erase(tt);
+        DP("Unregistering image " DPxMOD " from RTL " DPxMOD "!\n",
+            DPxPTR(img->ImageStart), DPxPTR(R->LibraryHandler));
----------------
I think this is only needed once per `desc` and not per `img`, isn't it?


================
Comment at: libomptarget/src/omptarget.cpp:1354-1355
+static int CheckDevice(int32_t device_id) {
+  // Get device info.
+  DeviceTy &Device = Devices[device_id];
+
----------------
The `Device` should only be fetched after the `device_id` has been proved valid


================
Comment at: libomptarget/src/omptarget.cpp:1357-1377
+  // No devices available?
+  // Devices.size() can only change while registering a new
+  // library, so try to acquire the lock of RTLs' mutex.
+  RTLsMtx.lock();
+  size_t Devices_size = Devices.size();
+  RTLsMtx.unlock();
+  if (!(device_id >= 0 && (size_t)device_id < Devices_size)) {
----------------
This looks the same as `device_is_ready`, can this be reused?


================
Comment at: libomptarget/src/omptarget.cpp:1383-1384
+  Device.PendingGlobalsMtx.unlock();
+  if (hasPendingGlobals) {
+    if (InitLibrary(Device) != OFFLOAD_SUCCESS) {
+      DP("Failed to init globals on device %d\n", device_id);
----------------
can be collapsed


================
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.
----------------
Backwards-compatible on the first check-in? That sounds weird...


================
Comment at: libomptarget/src/omptarget.cpp:2116
+            DPxPTR(HstPtrBegin));
+        rc = OFFLOAD_FAIL;
+      } else {
----------------
`break` afterwards?


================
Comment at: libomptarget/src/omptarget.cpp:2131
+            DP ("Copying data to device failed.\n");
+            rc = OFFLOAD_FAIL;
+          }
----------------
likewise?


================
Comment at: libomptarget/src/omptarget.h:86
+struct __tgt_bin_desc {
+  int32_t NumDevices;                // Number of device types supported
+  __tgt_device_image *DeviceImages;  // Array of device images (1 per dev. type)
----------------
Maybe rename this to `NumDeviceImages`?


================
Comment at: libomptarget/src/omptarget.h:88-89
+  __tgt_device_image *DeviceImages;  // Array of device images (1 per dev. type)
+  __tgt_offload_entry *EntriesBegin; // Begin of table with all host entries
+  __tgt_offload_entry *EntriesEnd;   // End of table (non inclusive)
+};
----------------
Can those be renamed to have `Host` in the same so that I'm not that much confused? :D


================
Comment at: libomptarget/src/omptarget.h:166
+// is non-zero after the region execution is done it also performs the
+// same action as data_update and data_end above. The following types are
+// used; this function returns 0 if it was able to transfer the execution
----------------
Only as `data_end`, not as `data_update`?


================
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()
----------------
1. This will abort CMake which probably isn't a good idea.
2. `MSVC` is possible for standalone builds?


https://reviews.llvm.org/D14031





More information about the Openmp-commits mailing list