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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
