[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 added a comment.

Please also `clang-format` your changes.



================
Comment at: libomptarget/src/interface.cpp:28-61
+// mutex
+std::mutex LibomptargetPrintMtx;
+
+
+////////////////////////////////////////////////////////////////////////////////
+/// support for print and assert
+
----------------
This seems overly complex where we now only need a single `fprintf` (which is thread-safe) inside an `if`. Do you have upcoming patches that will use these functions?


================
Comment at: libomptarget/src/interface.cpp:65
+/// manage the success or failure of a target constuct
+static void handle_target_outcome(bool success)
+{
----------------
Please rename to CamelCase, something like `HandleTargetOutcome`? (`device_is_ready` seems to be the exception)


================
Comment at: libomptarget/src/interface.cpp:67-77
+  if (TargetOffloadPolicy == tgt_default) {
+    if (Devices.size() > 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 "
----------------
After some thinking I guess this actually works in all cases:
1) If there is at least one device, the code path in `RTLsTy::LoadRTLs` will be executed.
2) Otherwise all functions check whether the device ID is less than `Devices.size()` (mostly in `device_is_ready` which is called from `CheckDeviceAndCtors`). This can never be true when there is no RTL and the execution ends up here.

However I don't find this very intuitive and it will be very easy to mess up with future changes. I suggest to do this upfront, for example with a helper function:
```lang=c++
static bool IsOffloadDisabled() {
  if (TargetOffloadPolicy == tgt_default) {
    // ...
  }
  return TargetOffloadPolicy == tgt_disabled;
}
```
The first line of each interface function would then become `if (IsOffloadDisabled()) return;`. In addition the duplicate code could be dropped from `RTLsTy::LoadRTLs`. Do you have concerns about this straight-forward solution?


Repository:
  rOMP OpenMP

https://reviews.llvm.org/D50522