[PATCH] D86119: [OPENMP50]Allow overlapping mapping in target constrcuts.

Wed Oct 14 10:52:40 PDT 2020

jdenny added a comment.

In D86119#2330339 <https://reviews.llvm.org/D86119#2330339>, @ABataev wrote:

> In D86119#2330310 <https://reviews.llvm.org/D86119#2330310>, @jdenny wrote:
>
>> Thanks for working on this.  Sorry to take so long to review.  Before I try to digest the code, I have a few high-level questions.
>>
>> Based on the test suite changes, `TARGET_PARAM` is disappearing from many cases.  Can you explain a bit how that supports overlapping and reordering?
>
> `TARGET_PARAM` is used only for marking the data that should be passed to the kernel function as an argument. We just generate it in many cases but the runtime actually does not use them. Thу patch relies on this thing, otherwise, we may pass an incorrect number of arguments to the kernel functions.

Is it reasonable to extract that change into a parent patch?  That would at least make the test suite changes easier to follow.

>> Have you considered issue 2337 for the OpenMP spec and how your implementation handles the ambiguous cases cited there?
>
> Can you provide the details about this issue?

It discusses ambiguities in the way the spec describes map clause ordering.  Here's one example relevant to `present`:

  #pragma omp target exit data map(from: x) map(present, release: x)

One statement in the spec says a map clause with `from` is effectively ordered before one with `release`.  Another statement says a map clause with `present` is effectively ordered before one without.  Which applies in the above example?  In some discussions, people agreed the statement about `present` was intended to have precedence, but the spec doesn't say that.  That resolution probably makes sense at entry to a region, but does it make sense at exit?  Would it suppress `from` in this example?  (Actually, should there be two reference counter decrements in this example or just one?)

These ambiguities are the reason I punted on this issue when implementing `present`.  If we can come up with a reasonable implementation for all cases, perhaps we can propose a new wording for the spec.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86119