[PATCH] D91373: [OpenMP5.0] Support more kinds of lvalues in map clauses
jacobdweightman added a comment. Ping CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91373/new/ https://reviews.llvm.org/D91373 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D91373: [OpenMP5.0] Support more kinds of lvalues in map clauses
jacobdweightman created this revision.
jacobdweightman added a reviewer: ABataev.
jacobdweightman added a project: OpenMP.
Herald added subscribers: cfe-commits, guansong, yaxunl.
Herald added a project: clang.
jacobdweightman requested review of this revision.
Herald added a reviewer: jdoerfert.
Herald added a subscriber: sstefan1.
This patch aims to fully support this change from OpenMP 4.5 to 5.0:
> The to and from clauses on the target update construct (see Section 2.12.6 on
> page 176), the depend clause on task generating constructs (see Section
> 2.17.11 on page 255), and the map clause (see Section 2.19.7.1 on page 315)
> were extended to allow any lvalue expression as a list item for C/C++.
Specifically, support is added for:
- calling functions which return references/constexprs
- cast expressions as part of a larger lvalue expression
- expressions with the ternary Elvis operators (A ? B : C and A ?: C)
This patch modifies Sema to accept such expressions. A few small changes in
CodeGen were required to avoid asserts, but CodeGen appeared to already handle
such expressions correctly.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D91373
Files:
clang/lib/CodeGen/CGOpenMPRuntime.cpp
clang/lib/Sema/SemaOpenMP.cpp
clang/test/OpenMP/target_map_messages.cpp
clang/test/OpenMP/target_parallel_for_map_messages.cpp
clang/test/OpenMP/target_parallel_for_simd_map_messages.cpp
clang/test/OpenMP/target_parallel_map_messages.cpp
clang/test/OpenMP/target_simd_map_messages.cpp
clang/test/OpenMP/target_teams_distribute_map_messages.cpp
clang/test/OpenMP/target_teams_distribute_parallel_for_map_messages.cpp
clang/test/OpenMP/target_teams_distribute_parallel_for_simd_map_messages.cpp
clang/test/OpenMP/target_teams_distribute_simd_map_messages.cpp
clang/test/OpenMP/target_teams_map_messages.cpp
clang/test/OpenMP/target_update_from_messages.cpp
clang/test/OpenMP/target_update_to_messages.cpp
Index: clang/test/OpenMP/target_update_to_messages.cpp
===
--- clang/test/OpenMP/target_update_to_messages.cpp
+++ clang/test/OpenMP/target_update_to_messages.cpp
@@ -1,12 +1,12 @@
-// RUN: %clang_cc1 -verify=expected,le50 -fopenmp -ferror-limit 100 %s -Wno-openmp-mapping -Wuninitialized
-// RUN: %clang_cc1 -verify=expected,le45 -fopenmp -fopenmp-version=40 -ferror-limit 100 %s -Wno-openmp-mapping -Wuninitialized
-// RUN: %clang_cc1 -verify=expected,le45 -fopenmp -fopenmp-version=45 -ferror-limit 100 %s -Wno-openmp-mapping -Wuninitialized
-// RUN: %clang_cc1 -verify=expected,le50 -fopenmp -fopenmp-version=50 -ferror-limit 100 %s -Wno-openmp-mapping -Wuninitialized
+// RUN: %clang_cc1 -verify=expected,le50 -fopenmp -ferror-limit 200 %s -Wno-openmp-mapping -Wuninitialized
+// RUN: %clang_cc1 -verify=expected,le45 -fopenmp -fopenmp-version=40 -ferror-limit 200 %s -Wno-openmp-mapping -Wuninitialized
+// RUN: %clang_cc1 -verify=expected,le45 -fopenmp -fopenmp-version=45 -ferror-limit 200 %s -Wno-openmp-mapping -Wuninitialized
+// RUN: %clang_cc1 -verify=expected,le50 -fopenmp -fopenmp-version=50 -ferror-limit 200 %s -Wno-openmp-mapping -Wuninitialized
-// RUN: %clang_cc1 -verify=expected,le50 -fopenmp-simd -ferror-limit 100 %s -Wno-openmp-mapping -Wuninitialized
-// RUN: %clang_cc1 -verify=expected,le45 -fopenmp-simd -fopenmp-version=40 -ferror-limit 100 %s -Wno-openmp-mapping -Wuninitialized
-// RUN: %clang_cc1 -verify=expected,le45 -fopenmp-simd -fopenmp-version=45 -ferror-limit 100 %s -Wno-openmp-mapping -Wuninitialized
-// RUN: %clang_cc1 -verify=expected,le50 -fopenmp-simd -fopenmp-version=50 -ferror-limit 100 %s -Wno-openmp-mapping -Wuninitialized
+// RUN: %clang_cc1 -verify=expected,le50 -fopenmp-simd -ferror-limit 200 %s -Wno-openmp-mapping -Wuninitialized
+// RUN: %clang_cc1 -verify=expected,le45 -fopenmp-simd -fopenmp-version=40 -ferror-limit 200 %s -Wno-openmp-mapping -Wuninitialized
+// RUN: %clang_cc1 -verify=expected,le45 -fopenmp-simd -fopenmp-version=45 -ferror-limit 200 %s -Wno-openmp-mapping -Wuninitialized
+// RUN: %clang_cc1 -verify=expected,le50 -fopenmp-simd -fopenmp-version=50 -ferror-limit 200 %s -Wno-openmp-mapping -Wuninitialized
void foo() {
}
@@ -15,6 +15,14 @@
return argc;
}
+int &id(int &x) {
+return x;
+}
+
+int *id2(int *x) {
+return x;
+}
+
struct S1; // expected-note 2 {{declared here}}
extern S1 a;
class S2 {
@@ -83,6 +91,12 @@
double marr[10][5][10];
#pragma omp target update to(marr [0:1][2:4][1:2]) // le45-error {{array section does not specify contiguous storage}} le45-error {{expected at least one 'to' clause or 'from' clause specified to '#pragma omp target update'}}
{}
+
+#pragma omp target update to(id) // le45-error {{expected expression containing only member accesses and/or array sections based on named variables}} le45-error {{expected at least one 'to' clause or 'from' clause specified to '#pragma omp target update'}} le50-error {{expe
[PATCH] D91373: [OpenMP5.0] Support more kinds of lvalues in map clauses
jacobdweightman added inline comments. Comment at: clang/lib/Sema/SemaOpenMP.cpp:17141 + Visit(ACO->getTrueExpr()->IgnoreParenImpCasts()) || + Visit(ACO->getFalseExpr()->IgnoreParenImpCasts()); + } It looks like this short-circuits and the false expression doesn't get visited if the true expression is "well-formed." We probably want to emit diagnostics and fail if either expression is bad (i.e. visit true expression && visit false expression), but that results in an incorrect component list. I suspect this may indicate a deeper issue, since I'm not sure what the components should actually be. It seems like in general it should be a tree-like structure rather than a list-like structure, but that seems like it may require significant changes. Any thoughts on this or suggestions for improvement? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91373/new/ https://reviews.llvm.org/D91373 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D91373: [OpenMP5.0] Support more kinds of lvalues in map clauses
jacobdweightman added a comment. Ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91373/new/ https://reviews.llvm.org/D91373 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D91373: [OpenMP5.0] Support more kinds of lvalues in map clauses
jacobdweightman added inline comments.
Comment at: clang/lib/Sema/SemaOpenMP.cpp:16869
+// Allow results of method calls to be mapped.
+if (isa(ME->getMemberDecl())) {
+ RelevantExpr = ME;
ABataev wrote:
> I don't think it should always return `true`. What about `map(s.foo)` where
> `foo` is a member function?
Hmm... I had previously added a test covering this on line 416 of
target_map_messages.cpp which seemed to be passing, but not for the reason I
thought. This program illustrates the difference:
```
struct Foo {
int x;
int &id() {
return x;
}
};
int x;
int &id() {
return x;
}
int main(void) {
Foo f;
#pragma omp target map(tofrom: id, f.id)
{}
}
```
The free function is parsed in `bool Parser::ParseOpenMPVarList` to this:
```
DeclRefExpr 0xfee3f8 'int &(void)' lvalue Function 0xfedc10 'id' 'int &(void)'
```
Whereas the method is parsed to this:
```
MemberExpr 0xfee438 '' .id 0xfeda00
`-DeclRefExpr 0xfee418 'struct Foo' lvalue Var 0xfede98 'f' 'struct Foo'
```
Note that the former is an lvalue, whereas the latter is not. Therefore, the
latter emits the error "early" inside of `void checkMappableExpressionList` due
to the `!RE->isLValue()` check near SemaOpenMP.cpp:17668 rather than in the
`Visit*` methods. I guess the current error message is misleading given that
`id` is still an lvalue, though. Perhaps it would be good to add a new message
specifically for free functions since they are lvalues, but that issue already
exists in Clang today and feels out of scope for this change. If you disagree,
I wouldn't mind adding it.
Comment at: clang/lib/Sema/SemaOpenMP.cpp:16939
+// forward to the source expression.
+return Visit(OVE->getSourceExpr()->IgnoreParenImpCasts());
+ }
ABataev wrote:
> Same, too general check, plus in some cases `OVE->getSourceExpr()` may return
> `nullptr`.
I'm not exactly sure in what sense this check is too general, but perhaps it
would be better to handle this together with the Elvis operator. For instance,
I could have a `VisitBinaryConditionalOperator` which would "unwrap" the
`OpaqueValueExpression` (OVA) directly rather than calling through to this
method, since this is the only context in which we expect to handle OVAs. Let
me know if this doesn't fully address your concern, though.
However, this usage of `getSourceExpr` appears to be consistent with other uses
of OVAs that I see in Clang (I see a few similar examples in the static
analyzer), but you make a good point. The Elvis operator's OVA should always
have a source expression which refers to the condition without the implicit
cast to bool. I'll add an assert that it isn't `nullptr` for now, but let me
know if you have any other ideas.
Comment at: clang/lib/Sema/SemaOpenMP.cpp:17072
bool VisitUnaryOperator(UnaryOperator *UO) {
-if (SemaRef.getLangOpts().OpenMP < 50 || !UO->isLValue() ||
-UO->getOpcode() != UO_Deref) {
+if (SemaRef.getLangOpts().OpenMP < 50 ||
+(Components.empty() &&
ABataev wrote:
> What is this for?
This is a bit of a hack and definitely unclear to the reader. The
`Components.empty()` check was added because an rvalue or a unary operator
other than the de-referencing operator should be allowed in a sub-expression of
the map clause list item, so long as the complete expression is an lvalue. As a
minimal example, consider something like `map(*(&x))`. More usefully, one may
cast pointer types so that a variable is mapped as a different type like
`map(*((int *) &x))`. Without this check, the new casting tests which do this
would fail.
A few ideas to make this more readable would be to use a temporary variable
before the `if` statement like `bool isFullExpression = Components.empty();` or
add a comment explaining the above. Do you have any other ideas for how to
improve this?
Comment at: clang/lib/Sema/SemaOpenMP.cpp:17115-17124
+ bool VisitCallExpr(CallExpr *CE) {
+if (SemaRef.getLangOpts().OpenMP < 50 ||
+(Components.empty() && !CE->isLValue())) {
+ emitErrorMsg();
+ return false;
+}
+assert(!RelevantExpr && "RelevantExpr is expected to be nullptr");
ABataev wrote:
> ```
> int a;
> int &foo() {return a;}
>
> ...
> #pragma omp target map(foo())
> foo() = 0;
> ...
>
> ```
> How is this supposed to work?
From my understanding of the spec, `foo` should be implicitly declared for both
the host and the target. However, the user would be responsible for explicitly
declaring `a` for the target if it isn't referenced in the `target` region.
This test program seems to behave as I expect, with the result that `a = 2`:
```
#include
int a;
#pragma omp declare target to(a)
int &foo() { return a; }
int main(void) {
a = 1;
#pragma omp target map(foo())
foo() = 2;
#pragma omp target update from(foo())
[PATCH] D91373: [OpenMP5.0] Support more kinds of lvalues in map clauses
jacobdweightman updated this revision to Diff 309056.
jacobdweightman added a comment.
I separated the handling of `ConditionalOperator`s and
`BinaryConditionalOperator`s in order to eliminate handling of
`OpaqueValueExpression`s in general, and asserted that the OVE's `SourceExpr`
is not null. I also made some minor changes to improve readability.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D91373/new/
https://reviews.llvm.org/D91373
Files:
clang/lib/CodeGen/CGOpenMPRuntime.cpp
clang/lib/Sema/SemaOpenMP.cpp
clang/test/OpenMP/target_map_messages.cpp
clang/test/OpenMP/target_parallel_for_map_messages.cpp
clang/test/OpenMP/target_parallel_for_simd_map_messages.cpp
clang/test/OpenMP/target_parallel_map_messages.cpp
clang/test/OpenMP/target_simd_map_messages.cpp
clang/test/OpenMP/target_teams_distribute_map_messages.cpp
clang/test/OpenMP/target_teams_distribute_parallel_for_map_messages.cpp
clang/test/OpenMP/target_teams_distribute_parallel_for_simd_map_messages.cpp
clang/test/OpenMP/target_teams_distribute_simd_map_messages.cpp
clang/test/OpenMP/target_teams_map_messages.cpp
clang/test/OpenMP/target_update_codegen.cpp
clang/test/OpenMP/target_update_from_messages.cpp
clang/test/OpenMP/target_update_to_messages.cpp
Index: clang/test/OpenMP/target_update_to_messages.cpp
===
--- clang/test/OpenMP/target_update_to_messages.cpp
+++ clang/test/OpenMP/target_update_to_messages.cpp
@@ -1,12 +1,12 @@
-// RUN: %clang_cc1 -verify=expected,le50 -fopenmp -ferror-limit 100 %s -Wno-openmp-mapping -Wuninitialized
-// RUN: %clang_cc1 -verify=expected,le45 -fopenmp -fopenmp-version=40 -ferror-limit 100 %s -Wno-openmp-mapping -Wuninitialized
-// RUN: %clang_cc1 -verify=expected,le45 -fopenmp -fopenmp-version=45 -ferror-limit 100 %s -Wno-openmp-mapping -Wuninitialized
-// RUN: %clang_cc1 -verify=expected,le50 -fopenmp -fopenmp-version=50 -ferror-limit 100 %s -Wno-openmp-mapping -Wuninitialized
+// RUN: %clang_cc1 -verify=expected,le50 -fopenmp -ferror-limit 200 %s -Wno-openmp-mapping -Wuninitialized
+// RUN: %clang_cc1 -verify=expected,le45 -fopenmp -fopenmp-version=40 -ferror-limit 200 %s -Wno-openmp-mapping -Wuninitialized
+// RUN: %clang_cc1 -verify=expected,le45 -fopenmp -fopenmp-version=45 -ferror-limit 200 %s -Wno-openmp-mapping -Wuninitialized
+// RUN: %clang_cc1 -verify=expected,le50 -fopenmp -fopenmp-version=50 -ferror-limit 200 %s -Wno-openmp-mapping -Wuninitialized
-// RUN: %clang_cc1 -verify=expected,le50 -fopenmp-simd -ferror-limit 100 %s -Wno-openmp-mapping -Wuninitialized
-// RUN: %clang_cc1 -verify=expected,le45 -fopenmp-simd -fopenmp-version=40 -ferror-limit 100 %s -Wno-openmp-mapping -Wuninitialized
-// RUN: %clang_cc1 -verify=expected,le45 -fopenmp-simd -fopenmp-version=45 -ferror-limit 100 %s -Wno-openmp-mapping -Wuninitialized
-// RUN: %clang_cc1 -verify=expected,le50 -fopenmp-simd -fopenmp-version=50 -ferror-limit 100 %s -Wno-openmp-mapping -Wuninitialized
+// RUN: %clang_cc1 -verify=expected,le50 -fopenmp-simd -ferror-limit 200 %s -Wno-openmp-mapping -Wuninitialized
+// RUN: %clang_cc1 -verify=expected,le45 -fopenmp-simd -fopenmp-version=40 -ferror-limit 200 %s -Wno-openmp-mapping -Wuninitialized
+// RUN: %clang_cc1 -verify=expected,le45 -fopenmp-simd -fopenmp-version=45 -ferror-limit 200 %s -Wno-openmp-mapping -Wuninitialized
+// RUN: %clang_cc1 -verify=expected,le50 -fopenmp-simd -fopenmp-version=50 -ferror-limit 200 %s -Wno-openmp-mapping -Wuninitialized
void foo() {
}
@@ -15,6 +15,14 @@
return argc;
}
+int &id(int &x) {
+ return x;
+}
+
+int *id2(int *x) {
+ return x;
+}
+
struct S1; // expected-note 2 {{declared here}}
extern S1 a;
class S2 {
@@ -83,6 +91,12 @@
double marr[10][5][10];
#pragma omp target update to(marr [0:1][2:4][1:2]) // le45-error {{array section does not specify contiguous storage}} le45-error {{expected at least one 'to' clause or 'from' clause specified to '#pragma omp target update'}}
{}
+
+#pragma omp target update to(id) // le45-error {{expected expression containing only member accesses and/or array sections based on named variables}} le45-error {{expected at least one 'to' clause or 'from' clause specified to '#pragma omp target update'}} le50-error {{expected addressable lvalue in 'to' clause}} le50-error {{expected at least one 'to' clause or 'from' clause specified to '#pragma omp target update'}}
+#pragma omp target update to(id(this->a)) // le45-error {{expected expression containing only member accesses and/or array sections based on named variables}} le45-error {{expected at least one 'to' clause or 'from' clause specified to '#pragma omp target update'}}
+#pragma omp target update to(id(id(this->a))) // le45-error {{expected expression containing only member accesses and/or array sections based on named variables}} le45-error {{expected at least one 'to' clause or 'from' clause specified to '#pragma omp target
