[Openmp-commits] [PATCH] D50522: [OpenMP][libomptarget] Bringing up to spec with respect to OMP_TARGET_OFFLOAD env var

[Jonas Hahnfeld via Phabricator via Openmp-commits]

Hahnfeld requested changes to this revision.
Hahnfeld added inline comments.
This revision now requires changes to proceed.


================
Comment at: libomptarget/include/omptarget.h:188-194
+/*
+ * To enable libomptarget debugging:
+ *   1) uncommend OMPTARGET_DEBUG #define
+ *   2) set LIBOMPTARGET_DEBUG environment variable before executing:
+ *      eg. export LIBOMPTARGET_DEBUG=1
+ */
+// #define OMPTARGET_DEBUG 1
----------------
Please remove, there is a CMake flag to do this.


================
Comment at: libomptarget/src/device.cpp:368-370
+
+// manage the success or failure of a target constuct
+void handle_target_outcome(bool success)
----------------
I think this implementation can live directly in `interface.cpp` as no other file should use it. Please make the function `static` and I think internal function names follow a CamelCase naming convention; `device_is_ready` seems to be the exception...


================
Comment at: libomptarget/src/device.cpp:385-386
+      if (! success) {
+        DP("failure of target construct when expecting to successfully offload");
+        assert(success);
+      }
----------------
Asserts are no-ops when building with `-DNDEBUG` which is the default for `Release` builds. Call `exit(1)` directly?

I think we should provide the user with an error message when aborting the execution, `DP` is subject to `LIBOMPTARGET_DEBUG` and only available in `Debug` builds.

(PGI's OpenACC implementation prints a detailed error code from CUDA which is probably not possible in the target agnostic part. I think that's ok for now, the user can use `cuda-gdb` or other tools...)


================
Comment at: libomptarget/src/device.h:24-31
+// enum for OMP_TARGET_OFFLOAD; keep in sync with kmp.h definition
+enum kmp_target_offload_kind {
+  tgt_disabled = 0,
+  tgt_default = 1,
+  tgt_mandatory = 2
+};
+typedef enum kmp_target_offload_kind kmp_target_offload_kind_t;
----------------
Is there a particular reason the code is put into `device.h` / `device.cpp`? IMO it's not related to device management.

For later reuse in API methods (if deemed necessary after the discussion on `omp-lang`) I think this should go into `private.h`.


================
Comment at: libomptarget/src/rtl.cpp:218-228
+        if (TargetOffloadPolicy == tgt_default) {
+          if (R.NumberOfDevices > 0) {
+            DP("Default TARGET OFFLOAD policy is now mandatory "
+                "(devicew were found)\n");
+            TargetOffloadPolicy = tgt_mandatory;
+          } else {
+            DP("Default TARGET OFFLOAD policy is now disabled "
----------------
I think this code path is not triggered when there is no matching RTL at all. At the moment this causes an `assert` (in `Debug` builds) because `TargetOffloadPolicy` is still `tgt_default`.

However I don't think we can decide on a single call to `__tgt_register_lib` for the reason you mentioned last week: The number of available devices can change when more device images are registered. As such I think we should move the decision handling `tgt_default` to the beginning of all interface methods that can come from a user's `target` construct. What do you think?


Repository:
  rOMP OpenMP

https://reviews.llvm.org/D50522