[Openmp-commits] [PATCH] D44522: [Libomptarget] Full implementation of the target-offload-icv

[Jonas Hahnfeld via Phabricator via Openmp-commits]

Hahnfeld added a comment.

This has completely gone under my radar. Be sure to ping changes if there is no review and some reviewers are actively working on other parts of LLVM.

In https://reviews.llvm.org/D44522#1039056, @RaviNarayanaswamy wrote:

> 21 The DEFAULT value specifies that when one or more target devices are available, the runtime
>  22 behaves as if this environment variable is set to MANDATORY; otherwise, the runtime behaves as if
>  23 this environment variable is set to DISABLED.


After reviewing the code (see inline comments) I don't see this behavior implemented: It only falls back to DISABLED, but doesn't raise to MANDATORY. I'm not sure though if there have been changes to the spec after Ravi posted the quote.

Another general comment: This patch doesn't handle API routines, I think they should be affected as well?



================
Comment at: libomptarget/src/interface.cpp:28-30
+    fprintf(stderr, "OMP: WARNING: Target offload failure, terminating "
+        "application\n");
+    exit(OFFLOAD_FAIL);
----------------
If we are exiting, this shouldn't say `WARNING`. I think the word `failure` is enough here?


================
Comment at: libomptarget/src/interface.cpp:33-35
+      fprintf(stderr, "OMP: WARNING: Target offload failure after offload "
+          "success, terminating application due to possible inconsistent state "
+          "of data\n");
----------------
Likewise


================
Comment at: libomptarget/src/interface.cpp:37
+      exit(OFFLOAD_FAIL);
+    } else { // if (!HasSuccessfulRuns)
+      fprintf(stderr, "OMP: WARNING: Target offload failure, falling back to "
----------------
Remove dead code


================
Comment at: libomptarget/src/interface.cpp:45-59
+#define CHECK_HOST_FALLBACK_DATA()                                             \
+  {                                                                            \
+    if (TargetOffloadKind == OMP_TGT_OFFLOAD_DISABLED) {                       \
+      DP("Target offload disabled, ignoring \"target data\", "                 \
+          "\"target enter/exit data\" or \"target update\"\n");                \
+      return;                                                                  \
+    }                                                                          \
----------------
I'd prefer eliminating these macros and just putting the check in all entries. This also has the advantage of having precise debugging output.


================
Comment at: libomptarget/src/omptarget.cpp:31-38
+// Variable that controls the behavior of libomptarget in case of offload
+// failure when the offload kind is DEFAULT. It is OK to always fail (we can
+// safely fall back to the host, since data on the host is up to date). It is
+// not OK to succeed at first and then fail, as the most up to date data might
+// be on the device, so falling back to the host is not guaranteed to yield
+// correct results. Once an offload has fallen back on the host, every
+// subsequent offload will also be executed on the host.
----------------
No need to have this here, just put it in `interface.cpp` and make it `static` (don't forget to remove the declaration from the header file!)


Repository:
  rOMP OpenMP

https://reviews.llvm.org/D44522