[Openmp-commits] [PATCH] D64626: [libomptarget] Handle offload policy in push_target_tripcount

[Jonas Hahnfeld via Phabricator via Openmp-commits]

Hahnfeld added a comment.

In D64626#1596295 <https://reviews.llvm.org/D64626#1596295>, @jdoerfert wrote:

> In D64626#1596246 <https://reviews.llvm.org/D64626#1596246>, @Hahnfeld wrote:
>
> > In D64626#1596237 <https://reviews.llvm.org/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`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64626/new/

https://reviews.llvm.org/D64626