[PATCH] D91373: [OpenMP5.0] Support more kinds of lvalues in map clauses
Alexey Bataev via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu Dec 10 09:35:26 PST 2020
ABataev added a comment.
Restore original formatting in test cases that were not directly affected by the patch. Also, I would start with a single kind of expression rather than trying to cover as many kinds of expressions as possible. It makes it easier to understand and to review it.
================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:16869
+ // Allow results of method calls to be mapped.
+ if (isa<CXXMethodDecl>(ME->getMemberDecl())) {
+ RelevantExpr = ME;
----------------
jacobdweightman wrote:
> 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 '<bound member function type>' .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.
The mapping of functions is meaningless here, if they should return references to the globals and these globals should be marked as declare target
================
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() &&
----------------
jacobdweightman wrote:
> 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?
I would say that something like `map(*((int *) &x))` should not be allowed. Something like `map(*p)` - yes, but not something that changes the original type.
================
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");
----------------
jacobdweightman wrote:
> 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 <stdio.h>
>
> 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())
>
> printf("a = %d\n", a);
> }
> ```
Just like I said before, then mapping of the function is absolutely meaningless, we can just ignore it.
================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:17125-17144
+ bool VisitCastExpr(CastExpr *CE) {
+ if (SemaRef.getLangOpts().OpenMP < 50 ||
+ (Components.empty() && !CE->isLValue())) {
+ emitErrorMsg();
+ return false;
+ }
+ assert(!RelevantExpr && "RelevantExpr is expected to be nullptr");
----------------
jacobdweightman wrote:
> ABataev wrote:
> > Same questions here, how's the actual mapping is supposed to work? Need some more detailed description. I don't think this is going to be easy to implement it directly. We map the addresses of the base declarations but in your cases there is just no base declarations. SO, you need to create one. To me, this should look like this:
> > ```
> > #pragma omp target map(<lvalue>)
> > {
> > <lvalue> = xxx;
> > yyy = <lvalue>;
> > }
> > ```
> > |
> > V
> > ```
> > auto &mapped = <lvalue>;
> > #pragma omp target map(mapped)
> > {
> > mapped = xxx;
> > yyy = mapped;
> > }
> > ```
> >
> I think there's an issue with that source-level transformation when evaluating `<lvalue>` has side effects, since they would be performed 3 times in the first program but only once in the second program.
>
> I might be off base here, but would there also be an issue if `<lvalue>` might share original storage with another list item? For example, shouldn't we issue an error when compiling the following program?
> ```
> int a[10], b[10];
> bool c;
> #pragma omp target map(a, c ? a[1] : b[1])
> { ... }
> ```
>
> If I'm missing something here, how would we go about implementing a transformation like that? Is it something that belongs in codegen? I think I need a bit more guidance here.
I assume side effects are not allowed throughout the standard though it is not explicitly expressed for the map clauses.
Plus, the fact that something is not allowed does not mean that the compiler should diagnose it. In general, it is just impossible, just in some cases.
================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:17150-17151
+ assert(TrueExpr && "Failed to resolve true expression of Elvis operator.");
+ return RelevantExpr || Visit(TrueExpr->IgnoreParenImpCasts()) ||
+ Visit(BCO->getFalseExpr()->IgnoreParenImpCasts());
+ }
----------------
`&&`?
================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:17343
+ // perform these checks on.
+ const Expr *nextComponentExpr = (CI == CE)
+ ? SI->getAssociatedExpression()
----------------
`NextComponentExpr`
================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:17346
+ : CI->getAssociatedExpression();
+ const bool shouldCheckForOverlap =
+ isa<ArraySubscriptExpr>(nextComponentExpr) ||
----------------
`ShouldCheckForOverlap `
================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:17350
+ isa<OMPArrayShapingExpr>(nextComponentExpr) ||
+ isa<MemberExpr>(nextComponentExpr);
+
----------------
What if MemberExpr is a base of array section, subscript etc.?
================
Comment at: clang/test/OpenMP/target_map_messages.cpp:95-114
+#pragma omp target map(to \
+ : ss) // expected-error {{threadprivate variables are not allowed in 'map' clause}}
{}
- #pragma omp target map(to:b,e)
+#pragma omp target map(to \
+ : b, e)
{}
----------------
Restore original formatting
================
Comment at: clang/test/OpenMP/target_map_messages.cpp:191-208
+ #pragma omp target map(, f, : a)
{}
- #pragma omp target map(always close: a) // expected-error {{missing map type}}
+ #pragma omp target map(always close: a) // expected-error {{missing map type}}
{}
- #pragma omp target map(always close bf: a) // expected-error {{incorrect map type, expected one of 'to', 'from', 'tofrom', 'alloc', 'release', or 'delete'}}
+ #pragma omp target map(always close bf: a) // expected-error {{incorrect map type, expected one of 'to', 'from', 'tofrom', 'alloc', 'release', or 'delete'}}
{}
// ge51-error at +3 {{incorrect map type modifier, expected 'always', 'close', 'mapper', or 'present'}}
----------------
Same here, remove unrelated changes
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D91373/new/
https://reviews.llvm.org/D91373
More information about the cfe-commits
mailing list