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

[Alexandre Eichenberger via Phabricator via Openmp-commits]

AlexEichenberger marked 2 inline comments as done.
AlexEichenberger added a comment.

responded to comments



================
Comment at: libomptarget/src/interface.cpp:28-61
+// mutex
+std::mutex LibomptargetPrintMtx;
+
+
+////////////////////////////////////////////////////////////////////////////////
+/// support for print and assert
+
----------------
Hahnfeld wrote:
> 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?
Yes, I intend this to be used for all output feedback to users. Ideally, I would find a way to share the infrastructure in kmpc, but for the moment this should do it. Note that the lock is only acquired prior to aborting, so overheads are not an issue.


================
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 "
----------------
Hahnfeld wrote:
> 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?
good suggestions, re-inserted the lock as now there could be a race condition. Again, in practice, this should not happen often. In practice, the lock should be acquired only once, so this is ok


Repository:
  rOMP OpenMP

https://reviews.llvm.org/D50522