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