[PATCH] D65835: [OpenMP] Fix map/is_device_ptr with DSA on combined directive

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

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

In D65835#1618865 <https://reviews.llvm.org/D65835#1618865>, @ABataev wrote:

> `is_device_ptr` can be considered as a kind of mapping clause (see 2.19.7   Data-Mapping Attribute Rules, Clauses, and Directives), so, I assume, clang correct here in terms of OpenMP 4.5.


Maybe, but I haven't found any statement in either version that states that map restrictions apply to is_device_ptr.

Another question is whether the restriction would make sense.  For example, does it ever make sense to specify both is_device_ptr and firstprivate for the same variable on a target construct?  I think that would mean that modifications that are made to that device pointer within the target region should not be seen after the target region.  I think that's reasonable, but that combination is not possible with the restriction.  As Johannes points out, private plus is_device_ptr probably doesn't make sense.

> Thus, I would not call this a "fix", this is just a new feature from OpenMP 5.0.

Understood.

I should have reported that the current implementation isn't complete for OpenMP 4.5.  For example, on `target teams`, `reduction(+:x) map(x)` is an error but not `map(x) reduction(+:x)`.  So there are bugs to fix, and maybe this will evolve into multiple patches, but I want to be sure I'm on the right path first.

> Plus, these changes are not enough to support this new feature from OpenMP 5.0. There definitely must be some changes in the codegen. If the variable is mapped in the target construct, we should not generate a code for the private clause of this variable on the target construct, since, in this case, private clauses are applied for the inner subdirectives, which are the part of the combined directive, but not the `target` part of the directive.

I'll look into it.

Thanks for the quick review.



================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:10895
+    // combined construct.
+    if (CurrDir == OMPD_target) {
       OpenMPClauseKind ConflictKind;
----------------
ABataev wrote:
> I would suggest to guard this change and limit this new functionality only for OpenMP 5.0. 
Do you agree that this is strictly an extension to 4.5 that won't alter the behavior of 4.5-conforming applications?

Do we generally want to complain about the use of extensions, or is there another reason for the guard you suggest?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65835