[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 3 inline comments as done.
AlexEichenberger added a comment.

responded to comments.



================
Comment at: libomptarget/src/interface.cpp:52
     DP("Failed to get device %" PRId64 " ready\n", device_id);
+    handle_target_outcome(false);
     return;
----------------
AlexEichenberger wrote:
> RaviNarayanaswamy wrote:
> > Why not move the check of handle_target_outcome into CheckDeviceAndCtors  so you dont have to do every time you invoke this function.
> Ravi, I did your change but reverted it for the following reason. I liked to have all of the handling of target_offload variable in the same file (interface.cpp). When I moved in into the CheckDeviceAndCtors, I needed to insert it in 2 places within the function, and handle_target_outcome was now in two different files.
> 
> If you feel strongly about your request, I will be happy to move the code into CheckDeviceAndCtors.
Made the handle_target_outcome static to the interface.cpp as suggested by Jonas


================
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 "
----------------
Hahnfeld wrote:
> 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?
Agreed, you are absolutely right


Repository:
  rOMP OpenMP

https://reviews.llvm.org/D50522