[libomptarget] Handle offload policy in push_target_tripcount

[Hahnfeld]

Summary

If the first target region in a program calls the push_target_tripcount
function, libomptarget didn't handle the offload policy correctly.
This could lead to unexpected error messages as seen in
http://lists.llvm.org/pipermail/openmp-dev/2019-June/002561.html

To solve this, add a check calling IsOffloadDisabled() as all other
entry points already do. If this method returns false, libomptarget
is effectively disabled.

[ABataev]

Could you provide a test?

[Hahnfeld]

In D64626#1582628, @ABataev wrote:

Could you provide a test?

Ah yes, forgot to mention: We don't have tests for OMP_TARGET_OFFLOAD and I don't think that's easily possible because the host plugins don't return errors.

[Hahnfeld]

ping

[jdoerfert]

I do not understand what caused the error you linked. Which part of the push_tripcount call is problematic? I would have expected get_default_device to return the host id when offload is disabled and the code to not choke one the host device.

[Hahnfeld]

In D64626#1596237, @jdoerfert wrote:

I do not understand what caused the error you linked. Which part of the push_tripcount call is problematic? I would have expected get_default_device to return the host id when offload is disabled and the code to not choke one the host device.

According to the spec omp_get_default_device returns default-device-var and I couldn't find a paragraph that implies special handling when offloading is disabled.

Consequently, omp_get_default_device always returns a "real" device and if CheckDeviceAndCtors fails (in the linked case, it was because the user didn't compile for the right architecture), the call to HandleTargetOutcome(false) will result in the error message as seen in the linked post.

[jdoerfert]

In D64626#1596246, @Hahnfeld wrote:

In D64626#1596237, @jdoerfert wrote:

I do not understand what caused the error you linked. Which part of the push_tripcount call is problematic? I would have expected get_default_device to return the host id when offload is disabled and the code to not choke one the host device.

According to the spec omp_get_default_device returns default-device-var and I couldn't find a paragraph that implies special handling when offloading is disabled.

Consequently, omp_get_default_device always returns a "real" device and if CheckDeviceAndCtors fails (in the linked case, it was because the user didn't compile for the right architecture), the call to HandleTargetOutcome(false) will result in the error message as seen in the linked post.

So why not change omp_get_default_device to return the host device number if offloading is disabled? My reasoning is: Why should "offload disabled" be different from "no offload", e.g., because there are no devices available.

[Hahnfeld]

In D64626#1596295, @jdoerfert wrote:

In D64626#1596246, @Hahnfeld wrote:

In D64626#1596237, @jdoerfert wrote:

I do not understand what caused the error you linked. Which part of the push_tripcount call is problematic? I would have expected get_default_device to return the host id when offload is disabled and the code to not choke one the host device.

According to the spec omp_get_default_device returns default-device-var and I couldn't find a paragraph that implies special handling when offloading is disabled.

Consequently, omp_get_default_device always returns a "real" device and if CheckDeviceAndCtors fails (in the linked case, it was because the user didn't compile for the right architecture), the call to HandleTargetOutcome(false) will result in the error message as seen in the linked post.

So why not change omp_get_default_device to return the host device number if offloading is disabled? My reasoning is: Why should "offload disabled" be different from "no offload", e.g., because there are no devices available.

So you propose to change the default-device-var ICV? Because at the moment, I think omp_get_default_device returns the value of OMP_DEFAULT_DEVICE or 0, regardless of whether the device exists or not. The user can only find out via omp_get_num_devices which will return the correct value 0 in both cases.

Sure, we can still change the ICV (although I'm not sure if the runtime library can overwrite its value with the one returned by omp_get_initial_device()), but we still don't need to call any of CheckDeviceAndCtors and friends, so we can short-circuit all of this by checking IsOffloadDisabled(). After all, setting OMP_TARGET_OFFLOAD=disabled should disable all of libomptarget.

[jdoerfert]

In D64626#1596347, @Hahnfeld wrote:

In D64626#1596295, @jdoerfert wrote:

In D64626#1596246, @Hahnfeld wrote:

In D64626#1596237, @jdoerfert wrote:

I do not understand what caused the error you linked. Which part of the push_tripcount call is problematic? I would have expected get_default_device to return the host id when offload is disabled and the code to not choke one the host device.

According to the spec omp_get_default_device returns default-device-var and I couldn't find a paragraph that implies special handling when offloading is disabled.

Consequently, omp_get_default_device always returns a "real" device and if CheckDeviceAndCtors fails (in the linked case, it was because the user didn't compile for the right architecture), the call to HandleTargetOutcome(false) will result in the error message as seen in the linked post.

So why not change omp_get_default_device to return the host device number if offloading is disabled? My reasoning is: Why should "offload disabled" be different from "no offload", e.g., because there are no devices available.

So you propose to change the default-device-var ICV? Because at the moment, I think omp_get_default_device returns the value of OMP_DEFAULT_DEVICE or 0, regardless of whether the device exists or not. The user can only find out via omp_get_num_devices which will return the correct value 0 in both cases.

(I might not fully grasp the situation here, if so, feel free to present me a bigger picture).

I actually did not try to suggest a change of any ICV value. Let me try to explain:

My thought was that the following conditions should more or less result in the same execution:

• offloading is disabled, e.g., through an environment variable,
• offloading is not possible, e.g., there is no device,
• offloading is not possible, e.g., there was a problem initializing the device.

Part of what confuses me now is:

I think omp_get_default_device returns the value of OMP_DEFAULT_DEVICE or 0, regardless of whether the device exists or not.

I would have thought that omp_get_default_device returns the host device id if there is no other one available.

[..] After all, setting OMP_TARGET_OFFLOAD=disabled should disable all of libomptarget.

I agree. That is why I think adding a conditional to __kmpc_push_target_tripcount is not a conceptual fix of the issue. (Maybe I am wrong though).
The "building blocks", or alternatively all entry points, of the library have to work fine when offloading is disabled. Fixing __kmpc_push_target_tripcount
looks like a triage of one entry point.

[Hahnfeld]

In D64626#1596837, @jdoerfert wrote:

In D64626#1596347, @Hahnfeld wrote:

In D64626#1596295, @jdoerfert wrote:

In D64626#1596246, @Hahnfeld wrote:

In D64626#1596237, @jdoerfert wrote:

I do not understand what caused the error you linked. Which part of the push_tripcount call is problematic? I would have expected get_default_device to return the host id when offload is disabled and the code to not choke one the host device.

According to the spec omp_get_default_device returns default-device-var and I couldn't find a paragraph that implies special handling when offloading is disabled.

Consequently, omp_get_default_device always returns a "real" device and if CheckDeviceAndCtors fails (in the linked case, it was because the user didn't compile for the right architecture), the call to HandleTargetOutcome(false) will result in the error message as seen in the linked post.

So why not change omp_get_default_device to return the host device number if offloading is disabled? My reasoning is: Why should "offload disabled" be different from "no offload", e.g., because there are no devices available.

So you propose to change the default-device-var ICV? Because at the moment, I think omp_get_default_device returns the value of OMP_DEFAULT_DEVICE or 0, regardless of whether the device exists or not. The user can only find out via omp_get_num_devices which will return the correct value 0 in both cases.

(I might not fully grasp the situation here, if so, feel free to present me a bigger picture).

I actually did not try to suggest a change of any ICV value. Let me try to explain:

My thought was that the following conditions should more or less result in the same execution:

• offloading is disabled, e.g., through an environment variable,
• offloading is not possible, e.g., there is no device,
• offloading is not possible, e.g., there was a problem initializing the device.

Agreed, and it already does.

Part of what confuses me now is:

I think omp_get_default_device returns the value of OMP_DEFAULT_DEVICE or 0, regardless of whether the device exists or not.

I would have thought that omp_get_default_device returns the host device id if there is no other one available.

No, as I described above, the routine only returns the value of the ICV. This may be a device that doesn't exist, but omp_get_default_device doesn't care for this case.

[..] After all, setting OMP_TARGET_OFFLOAD=disabled should disable all of libomptarget.

I agree. That is why I think adding a conditional to __kmpc_push_target_tripcount is not a conceptual fix of the issue. (Maybe I am wrong though).
The "building blocks", or alternatively all entry points, of the library have to work fine when offloading is disabled. Fixing __kmpc_push_target_tripcount
looks like a triage of one entry point.

All other entry points have this guard, __kmpc_push_target_tripcount is the only one where it's missing. Maybe I should have stated this upfront, sorry.

[jdoerfert]

All other entry points have this guard, __kmpc_push_target_tripcount is the only one where it's missing. Maybe I should have stated this upfront, sorry.

That I did not know.

LGTM

openmp/libomptarget/src/interface.cpp

if (depNum + noAliasDepNum > 0)
     __kmpc_omp_taskwait(NULL, 0);
 
   return __tgt_target_teams(device_id, host_ptr, arg_num, args_base, args,
                             arg_sizes, arg_types, team_num, thread_limit);
 }
 
 EXTERN void __kmpc_push_target_tripcount(int64_t device_id,
     uint64_t loop_tripcount) {
+  if (IsOffloadDisabled())
+    return;
+
   if (device_id == OFFLOAD_DEVICE_DEFAULT) {
     device_id = omp_get_default_device();
   }
 
   if (CheckDeviceAndCtors(device_id) != OFFLOAD_SUCCESS) {
     DP("Failed to get device %" PRId64 " ready\n", device_id);
     HandleTargetOutcome(false);
     return;