[PATCH] D84422: [OpenMP] Fix `present` for exit from `omp target data`

[Joel E. Denny via Phabricator via llvm-commits]

jdenny marked an inline comment as done.
jdenny added a comment.

In D84422#2173449 <https://reviews.llvm.org/D84422#2173449>, @RaviNarayanaswamy wrote:

> In D84422#2173372 <https://reviews.llvm.org/D84422#2173372>, @jdenny wrote:
>
> > In D84422#2172898 <https://reviews.llvm.org/D84422#2172898>, @jdenny wrote:
> >
> > > Has anyone clarified the motivation for this behavior?
> >
> >
> > I meant, is there any insight into why the spec specifies this behavior?
> >
> > In D84422#2172926 <https://reviews.llvm.org/D84422#2172926>, @grokos wrote:
> >
> > > Instead of introducing new API functions and making all these changes in all these files, wouldn't it be easier if we just unset the `PRESENT` flag from arg_types in clang when we generate the call to `__tgt_target_data_end_*` if we are exiting from a scoped environment?
> >
> >
> > Ah, that does sound simpler.  Thanks.  I'll look into it.
> >
> > Suppressing the presence check on exit from `omp target` would require a runtime change in addition to the Clang change you suggest for `omp target data`.  However, I've so far failed to formulate a reasonable test case.  Specifically, I don't yet see a way to guarantee that the data will definitely be present at the start of `omp target` but might not be present by the end.  Is it possible?  If not, then maybe we should leave the check in place for `omp target`.
>
>
> I would rather not have a check if not required by the spec as it would an unnecessary overhead to performance.


I've added a comment to the runtime code that performs the check.  As you can see, the check is performed regardless.  It's just a question of whether the runtime treats it as an error.  I don't think performance is an issue.

My concern here is that it will be hard to justify changes to the runtime if I cannot formulate a use case.



================
Comment at: openmp/libomptarget/src/omptarget.cpp:511
+      // "omp target exit data" but not upon exiting an "omp target data".
+      if (HasPresentModifier && for_exit_data) {
         MESSAGE("device mapping required by 'present' map type modifier does "
----------------
This is where the runtime performs the check.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84422