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

[George Rokos via Phabricator via Openmp-commits]

grokos marked 7 inline comments as done.
grokos added a comment.

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.



================
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
----------------
Hahnfeld wrote:
> 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
OK, I moved them.


================
Comment at: libomptarget/src/omptarget.cpp:513
+
+  DeviceTy& Device = Devices[device_num];
+  long IsLast; // not used
----------------
Hahnfeld wrote:
> 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?
Good catch! I added a check.


================
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.
----------------
Hahnfeld wrote:
> 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?
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.


================
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);
----------------
Hahnfeld wrote:
> I think this has not yet been collapsed...
Done.


https://reviews.llvm.org/D14031