[PATCH] D91373: [OpenMP5.0] Support more kinds of lvalues in map clauses

[Jacob Weightman via Phabricator via cfe-commits]

jacobdweightman added inline comments.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:16869
+    // Allow results of method calls to be mapped.
+    if (isa<CXXMethodDecl>(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 '<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.


================
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 <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);
}
```


================
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");
----------------
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.


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

https://reviews.llvm.org/D91373