[PATCH] D72811: [WIP][OPENMP5.0] allow lvalue for motion clause

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:15293
+  //  TypeDecorCnt for B: 0
+  if (Depth <= TypeDecorCnt) {
+RelevantExpr = cast(DRE);

cchen wrote:
> ABataev wrote:
> > The check is really bad. If you want to handle something like `*(B+x)` 
> > better to treat, say, `B` as the base and `x` as the index and, thus, treat 
> > the whole expression as something like `B[x]`
> I don't understand how to do this, is there any sample code that I can learn 
> from? Thanks
Not sure we have something like this, need to invent something new if we want 
to support lvalues in full. Or discard it as unsupported if we're unable to 
support it properly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72811



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72811: [WIP][OPENMP5.0] allow lvalue for motion clause

[Chi Chun Chen via Phabricator via cfe-commits]

cchen marked an inline comment as done.
cchen added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:15293
+  //  TypeDecorCnt for B: 0
+  if (Depth <= TypeDecorCnt) {
+RelevantExpr = cast(DRE);

ABataev wrote:
> The check is really bad. If you want to handle something like `*(B+x)` better 
> to treat, say, `B` as the base and `x` as the index and, thus, treat the 
> whole expression as something like `B[x]`
I don't understand how to do this, is there any sample code that I can learn 
from? Thanks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72811



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72811: [WIP][OPENMP5.0] allow lvalue for motion clause

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

I would suggest starting with the reworking of the existing implementation with 
StmtVisitor class and after that step-by-step extend the functionality of the 
visitor with handling other kinds а expressions.




Comment at: clang/lib/Sema/SemaOpenMP.cpp:15274-15276
+  const std::string TypeStr = PT->getCanonicalTypeInternal().getAsString();
+  size_t TypeDecorCnt = std::count_if(TypeStr.begin(), TypeStr.end(),
+ [](char c) { return c == '*' || c == '['; });

Again, this definitely should be reworked.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:15293
+  //  TypeDecorCnt for B: 0
+  if (Depth <= TypeDecorCnt) {
+RelevantExpr = cast(DRE);

The check is really bad. If you want to handle something like `*(B+x)` better 
to treat, say, `B` as the base and `x` as the index and, thus, treat the whole 
expression as something like `B[x]`



Comment at: clang/lib/Sema/SemaOpenMP.cpp:15303
+  bool VisitMemberExpr(MemberExpr *ME) {
+Expr *E = cast(ME);
+Expr *BaseE = ME->getBase()->IgnoreParenImpCasts();

Why do you need this?



Comment at: clang/lib/Sema/SemaOpenMP.cpp:15338
+return false;
+  return Visit(E->IgnoreParenImpCasts());;
+}

Do not call `IgnoreParenImpCasts()` here, just `Visit(E)`. Also, I assume, it 
may lead to infinite recursion.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:15356
   }
+  return Visit(E->IgnoreParenImpCasts());;
+}

Same here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72811



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72811: [WIP][OPENMP5.0] allow lvalue for motion clause

[Chi Chun Chen via Phabricator via cfe-commits]

cchen marked an inline comment as done.
cchen added a comment.

> Quoted Text






Comment at: clang/lib/Sema/SemaOpenMP.cpp:15554
+LocatorChecker Checker;
+if (Checker.Visit(OrigExpr)) {
+  llvm::copy(Checker.getComponents(),

ABataev wrote:
> cchen wrote:
> > ABataev wrote:
> > > cchen wrote:
> > > > ABataev wrote:
> > > > > General question about several cases. How we're going to support them?
> > > > > 1. (a ? b : c). 
> > > > > 2. __builtin_choose_expr(a, b, c).
> > > > > 3. a = b.
> > > > > 4. a?:b
> > > > > 5. __builtin_convertvector(x, ty)
> > > > > 6. (int&)a
> > > > > 7. v.xy, where v is an extended vector
> > > > > 8. a.b, where b is a bitfield
> > > > > 9. __builtin_bit_cast(v, ty)
> > > > > 10. const_cast(a)
> > > > > 11. dynamic_cast(a)
> > > > > 12. reinterpret_cast
> > > > > 13. static_cast
> > > > > 14. typeid() (also is an lvalue).
> > > > > 15. __uuidof(*comPtr)
> > > > > 16. lambda calls
> > > > > 17. User defined literals
> > > > > 
> > > > I think we could first evaluate the lvalue, and then pass the result of 
> > > > `` to device? 
> > > What do you mean when you say `evaluate lvalue`? In veneral, it can be 
> > > evaluated at the runtime. Do you propose to implement dyic translation? 
> > > It will definetely slow down the execution on the device.
> > I mean evaluate lvalue before sending  to device. For example:
> > ```
> > int a, b;
> > b = 5;
> > #pragma omp target map(a = b) // assign b to a before sending `&(a=b)` to 
> > device
> > {
> >a++;
> > }
> > ```
> > Therefore, the above example have the same semantics as this:
> > ```
> > int a, b;
> > b = 5;
> > int  = (a=b)
> > #pragma omp target map(c)
> > {
> >a++;
> > }
> > ```
> Sure, we do this already, generally speaking. The main problem here is how to 
> map the address and associate it with address on the device. We do it at the 
> compile time, support of general lvalues requires runtime translation + 
> special instrumentation of the device code for address translation.
  - (a ? b : c).
- Error out for now, not sure what to for this one
  - __builtin_choose_expr(a, b, c).
- Allow this one since we know what to send to device at compile time
  - a = b.
   - Sema accept this one, but need more work for codegen I think (Clang not 
emit the a=b ir if "a=b" inside map/motion clause)
  - a?:b
- What's the name of this kind of expression? I don't know what to do for 
this one
  - __builtin_convertvector(x, ty)
- not sure
  - v.xy, where v is an extended vector
- not sure
  - a.b, where b is a bitfield
- Error out for this one since bitfield is not addressable
  - __builtin_bit_cast(v, ty)
- Error out for this one since bitfield is not addressable
  - casts
- Error out if the expression inside the cast is not lvalue
  - typeid() (also is an lvalue).
- I guess we can support this just like normal variable?
  - __uuidof(*comPtr)
- not sure
  - lambda calls
- OpenMP 5.0 spec has rules for lambda. Now not error out for sema but need 
more work for codegen
  - User defined literals
 - error out since not addressable



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72811



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72811: [WIP][OPENMP5.0] allow lvalue for motion clause

[Chi Chun Chen via Phabricator via cfe-commits]

cchen updated this revision to Diff 243950.
cchen added a comment.

1. Refactor based on feedback and use the visitor class to integrate the 
analysis

in checkMapClauseExpressionBase.

2. Handle more lvalue cases


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72811

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/lib/Sema/SemaOpenMP.cpp
  clang/test/OpenMP/target_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,6 +1,12 @@
-// RUN: %clang_cc1 -verify -fopenmp -ferror-limit 100 %s -Wno-openmp-mapping -Wuninitialized
+// RUN: %clang_cc1 -verify=expected,le45 -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 -fopenmp-simd -ferror-limit 100 %s -Wno-openmp-mapping -Wuninitialized
+// RUN: %clang_cc1 -verify=expected,le45 -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
 
 void foo() {
 }
@@ -59,6 +65,29 @@
   double *p;
   unsigned bfa : 4;
 };
+struct S8 {
+  int *ptr;
+  int a;
+  struct S7* S;
+  void foo() {
+#pragma omp target update to(*this) // 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(*(&(*this))) // 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(*(this->ptr)) // 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(*(&(*(this->S->i+this->S->p // 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(*(&(*(this->S->i+this->S->s6[0].pp // 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(*(&(*(a+this->ptr // 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(*(&(*(*(this->ptr)+a+this->ptr // 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'}}
+{}
+  }
+};
+
+struct Base {
+  virtual ~Base() {}
+};
+struct Derived: Base {
+  virtual void name() {}
+};
 
 S3 h;
 #pragma omp threadprivate(h) // expected-note 2 {{defined as threadprivate or thread local}}
@@ -89,12 +118,12 @@
 #pragma omp target update to(x)
 #pragma omp target update to(t[:I])
 #pragma omp target update to(T) // expected-error {{'T' does not refer to a value}} expected-error {{expected at least one 'to' clause or 'from' clause specified to '#pragma omp target update'}}
-#pragma omp target update to(I) // expected-error 2 {{expected expression containing only member accesses and/or array sections based on named variables}}
+#pragma omp target update to(I) // le50-error 2 {{expected lvalue with no 

[PATCH] D72811: [WIP][OPENMP5.0] allow lvalue for motion clause

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:15554
+LocatorChecker Checker;
+if (Checker.Visit(OrigExpr)) {
+  llvm::copy(Checker.getComponents(),

cchen wrote:
> ABataev wrote:
> > cchen wrote:
> > > ABataev wrote:
> > > > General question about several cases. How we're going to support them?
> > > > 1. (a ? b : c). 
> > > > 2. __builtin_choose_expr(a, b, c).
> > > > 3. a = b.
> > > > 4. a?:b
> > > > 5. __builtin_convertvector(x, ty)
> > > > 6. (int&)a
> > > > 7. v.xy, where v is an extended vector
> > > > 8. a.b, where b is a bitfield
> > > > 9. __builtin_bit_cast(v, ty)
> > > > 10. const_cast(a)
> > > > 11. dynamic_cast(a)
> > > > 12. reinterpret_cast
> > > > 13. static_cast
> > > > 14. typeid() (also is an lvalue).
> > > > 15. __uuidof(*comPtr)
> > > > 16. lambda calls
> > > > 17. User defined literals
> > > > 
> > > I think we could first evaluate the lvalue, and then pass the result of 
> > > `` to device? 
> > What do you mean when you say `evaluate lvalue`? In veneral, it can be 
> > evaluated at the runtime. Do you propose to implement dyic translation? It 
> > will definetely slow down the execution on the device.
> I mean evaluate lvalue before sending  to device. For example:
> ```
> int a, b;
> b = 5;
> #pragma omp target map(a = b) // assign b to a before sending `&(a=b)` to 
> device
> {
>a++;
> }
> ```
> Therefore, the above example have the same semantics as this:
> ```
> int a, b;
> b = 5;
> int  = (a=b)
> #pragma omp target map(c)
> {
>a++;
> }
> ```
Sure, we do this already, generally speaking. The main problem here is how to 
map the address and associate it with address on the device. We do it at the 
compile time, support of general lvalues requires runtime translation + special 
instrumentation of the device code for address translation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72811



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72811: [WIP][OPENMP5.0] allow lvalue for motion clause

[Chi Chun Chen via Phabricator via cfe-commits]

cchen marked an inline comment as done.
cchen added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:15554
+LocatorChecker Checker;
+if (Checker.Visit(OrigExpr)) {
+  llvm::copy(Checker.getComponents(),

ABataev wrote:
> cchen wrote:
> > ABataev wrote:
> > > General question about several cases. How we're going to support them?
> > > 1. (a ? b : c). 
> > > 2. __builtin_choose_expr(a, b, c).
> > > 3. a = b.
> > > 4. a?:b
> > > 5. __builtin_convertvector(x, ty)
> > > 6. (int&)a
> > > 7. v.xy, where v is an extended vector
> > > 8. a.b, where b is a bitfield
> > > 9. __builtin_bit_cast(v, ty)
> > > 10. const_cast(a)
> > > 11. dynamic_cast(a)
> > > 12. reinterpret_cast
> > > 13. static_cast
> > > 14. typeid() (also is an lvalue).
> > > 15. __uuidof(*comPtr)
> > > 16. lambda calls
> > > 17. User defined literals
> > > 
> > I think we could first evaluate the lvalue, and then pass the result of 
> > `` to device? 
> What do you mean when you say `evaluate lvalue`? In veneral, it can be 
> evaluated at the runtime. Do you propose to implement dyic translation? It 
> will definetely slow down the execution on the device.
I mean evaluate lvalue before sending  to device. For example:
```
int a, b;
b = 5;
#pragma omp target map(a = b) // assign b to a before sending `&(a=b)` to device
{
   a++;
}
```
Therefore, the above example have the same semantics as this:
```
int a, b;
b = 5;
int  = (a=b)
#pragma omp target map(c)
{
   a++;
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72811



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72811: [WIP][OPENMP5.0] allow lvalue for motion clause

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:15554
+LocatorChecker Checker;
+if (Checker.Visit(OrigExpr)) {
+  llvm::copy(Checker.getComponents(),

cchen wrote:
> ABataev wrote:
> > General question about several cases. How we're going to support them?
> > 1. (a ? b : c). 
> > 2. __builtin_choose_expr(a, b, c).
> > 3. a = b.
> > 4. a?:b
> > 5. __builtin_convertvector(x, ty)
> > 6. (int&)a
> > 7. v.xy, where v is an extended vector
> > 8. a.b, where b is a bitfield
> > 9. __builtin_bit_cast(v, ty)
> > 10. const_cast(a)
> > 11. dynamic_cast(a)
> > 12. reinterpret_cast
> > 13. static_cast
> > 14. typeid() (also is an lvalue).
> > 15. __uuidof(*comPtr)
> > 16. lambda calls
> > 17. User defined literals
> > 
> I think we could first evaluate the lvalue, and then pass the result of 
> `` to device? 
What do you mean when you say `evaluate lvalue`? In veneral, it can be 
evaluated at the runtime. Do you propose to implement dyic translation? It will 
definetely slow down the execution on the device.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72811



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72811: [WIP][OPENMP5.0] allow lvalue for motion clause

[Chi Chun Chen via Phabricator via cfe-commits]

cchen marked an inline comment as done.
cchen added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:15554
+LocatorChecker Checker;
+if (Checker.Visit(OrigExpr)) {
+  llvm::copy(Checker.getComponents(),

ABataev wrote:
> General question about several cases. How we're going to support them?
> 1. (a ? b : c). 
> 2. __builtin_choose_expr(a, b, c).
> 3. a = b.
> 4. a?:b
> 5. __builtin_convertvector(x, ty)
> 6. (int&)a
> 7. v.xy, where v is an extended vector
> 8. a.b, where b is a bitfield
> 9. __builtin_bit_cast(v, ty)
> 10. const_cast(a)
> 11. dynamic_cast(a)
> 12. reinterpret_cast
> 13. static_cast
> 14. typeid() (also is an lvalue).
> 15. __uuidof(*comPtr)
> 16. lambda calls
> 17. User defined literals
> 
I think we could first evaluate the lvalue, and then pass the result of 
`` to device? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72811



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72811: [WIP][OPENMP5.0] allow lvalue for motion clause

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:15201
+namespace {
+class LocatorChecker final : public StmtVisitor {
+  OMPClauseMappableExprCommon::MappableExprComponentList Components;

Seems to me, you're skipping many expressions, which could be supported. Did 
you try to test it with casting expressions and others?



Comment at: clang/lib/Sema/SemaOpenMP.cpp:15203-15205
+  DeclRefExpr *DRE;
+  CXXThisExpr *CTE;
+  size_t DerefCnt;

Use default initializers for fields.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:15213
+  bool VisitDeclRefExpr(DeclRefExpr *E) {
+if (const auto *VD = dyn_cast(E->getDecl())) {
+  const clang::Type *PT = E->getType().getTypePtr();

Better to use early exit where possible to reduce structural complexity of the 
functions.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:15215-15216
+  const clang::Type *PT = E->getType().getTypePtr();
+  const std::string TypeStr = PT->getCanonicalTypeInternal().getAsString();
+  size_t PtrCnt = std::count(TypeStr.begin(), TypeStr.end(), '*');
+  // Normally we want to add Components if DerefCnt == PtrCnt, for example:

This really looks ugly. Need to find a better solution.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:15233
+  //  PtrCnt for B: 0
+  if (DerefCnt <= PtrCnt) {
+pushComponentIfNoBaseDecl(E, E->getDecl());

Why do you need to count the number of dereferences? And have this comparison 
here?



Comment at: clang/lib/Sema/SemaOpenMP.cpp:15368
 
 if (auto *CurE = dyn_cast(E)) {
+  if (!(isa(CurE->getDecl(

If you have the visitor class, you can replace all this code with an analysis 
in the visitor.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:15369
 if (auto *CurE = dyn_cast(E)) {
-  if (!isa(CurE->getDecl()))
+  if (!(isa(CurE->getDecl(
 return nullptr;

Restore original code



Comment at: clang/lib/Sema/SemaOpenMP.cpp:15554
+LocatorChecker Checker;
+if (Checker.Visit(OrigExpr)) {
+  llvm::copy(Checker.getComponents(),

General question about several cases. How we're going to support them?
1. (a ? b : c). 
2. __builtin_choose_expr(a, b, c).
3. a = b.
4. a?:b
5. __builtin_convertvector(x, ty)
6. (int&)a
7. v.xy, where v is an extended vector
8. a.b, where b is a bitfield
9. __builtin_bit_cast(v, ty)
10. const_cast(a)
11. dynamic_cast(a)
12. reinterpret_cast
13. static_cast
14. typeid() (also is an lvalue).
15. __uuidof(*comPtr)
16. lambda calls
17. User defined literals




Comment at: clang/lib/Sema/SemaOpenMP.cpp:1
+if (Checker.Visit(OrigExpr)) {
+  llvm::copy(Checker.getComponents(),
+ std::back_inserter(CurComponents));

Better to pass `CurComponents` as an argument at the construction of the 
checker to avoid extra filling/copying.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72811



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72811: [WIP][OPENMP5.0] allow lvalue for motion clause

[Chi Chun Chen via Phabricator via cfe-commits]

cchen updated this revision to Diff 241293.
cchen added a comment.

Update due to negligence


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72811

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/lib/Sema/SemaOpenMP.cpp
  clang/test/OpenMP/target_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,6 +1,12 @@
-// RUN: %clang_cc1 -verify -fopenmp -ferror-limit 100 %s -Wno-openmp-mapping -Wuninitialized
+// RUN: %clang_cc1 -verify=expected,le45 -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 -fopenmp-simd -ferror-limit 100 %s -Wno-openmp-mapping -Wuninitialized
+// RUN: %clang_cc1 -verify=expected,le45 -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
 
 void foo() {
 }
@@ -59,6 +65,22 @@
   double *p;
   unsigned bfa : 4;
 };
+struct S8 {
+  int *ptr;
+  int a;
+  struct S7* S;
+  void foo() {
+#pragma omp target update to(*this) // 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(*(&(*this))) // 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(*(this->ptr)) // 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(*(&(*(this->S->i+this->S->p // 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(*(&(*(this->S->i+this->S->s6[0].pp // 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(*(&(*(a+this->ptr // 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(*(&(*(*(this->ptr)+a+this->ptr // 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'}}
+{}
+  }
+};
 
 S3 h;
 #pragma omp threadprivate(h) // expected-note 2 {{defined as threadprivate or thread local}}
@@ -89,12 +111,12 @@
 #pragma omp target update to(x)
 #pragma omp target update to(t[:I])
 #pragma omp target update to(T) // expected-error {{'T' does not refer to a value}} expected-error {{expected at least one 'to' clause or 'from' clause specified to '#pragma omp target update'}}
-#pragma omp target update to(I) // expected-error 2 {{expected expression containing only member accesses and/or array sections based on named variables}}
+#pragma omp target update to(I) // le50-error 2 {{expected lvalue with no function call in '#pragma omp target update' and '#pragma omp target map'}} le45-error 2 {{expected expression containing only member accesses and/or array sections based on named variables}}
 #pragma omp target 

[PATCH] D72811: [WIP][OPENMP5.0] allow lvalue for motion clause

[Chi Chun Chen via Phabricator via cfe-commits]

cchen updated this revision to Diff 241291.
cchen added a comment.

Fix based on feedback


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72811

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/lib/Sema/SemaOpenMP.cpp
  clang/test/OpenMP/target_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,6 +1,12 @@
-// RUN: %clang_cc1 -verify -fopenmp -ferror-limit 100 %s -Wno-openmp-mapping -Wuninitialized
+// RUN: %clang_cc1 -verify=expected,le45 -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 -fopenmp-simd -ferror-limit 100 %s -Wno-openmp-mapping -Wuninitialized
+// RUN: %clang_cc1 -verify=expected,le45 -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
 
 void foo() {
 }
@@ -59,6 +65,22 @@
   double *p;
   unsigned bfa : 4;
 };
+struct S8 {
+  int *ptr;
+  int a;
+  struct S7* S;
+  void foo() {
+#pragma omp target update to(*this) // 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(*(&(*this))) // 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(*(this->ptr)) // 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(*(&(*(this->S->i+this->S->p // 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(*(&(*(this->S->i+this->S->s6[0].pp // 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(*(&(*(a+this->ptr // 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(*(&(*(*(this->ptr)+a+this->ptr // 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'}}
+{}
+  }
+};
 
 S3 h;
 #pragma omp threadprivate(h) // expected-note 2 {{defined as threadprivate or thread local}}
@@ -89,12 +111,12 @@
 #pragma omp target update to(x)
 #pragma omp target update to(t[:I])
 #pragma omp target update to(T) // expected-error {{'T' does not refer to a value}} expected-error {{expected at least one 'to' clause or 'from' clause specified to '#pragma omp target update'}}
-#pragma omp target update to(I) // expected-error 2 {{expected expression containing only member accesses and/or array sections based on named variables}}
+#pragma omp target update to(I) // le50-error 2 {{expected lvalue with no function call in '#pragma omp target update' and '#pragma omp target map'}} le45-error 2 {{expected expression containing only member accesses and/or array sections based on named variables}}
 #pragma omp target update 

[PATCH] D72811: [WIP][OPENMP5.0] allow lvalue for motion clause

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D72811#1843066 , @cchen wrote:

> In D72811#1839561 , @ABataev wrote:
>
> > In D72811#1839538 , @cchen wrote:
> >
> > > In D72811#1837392 , @jdoerfert 
> > > wrote:
> > >
> > > > Thanks for working on this! While you are at it, `*this` is probably 
> > > > one of the most important ones to test and support.
> > >
> > >
> > > Can anyone tell me how to get `ValueDecl` from `CXXThisExpr`? I cannot 
> > > find any method return decl in 
> > > https://clang.llvm.org/doxygen/classclang_1_1CXXThisExpr.html#details.
> > >  Thanks!
> >
> >
> > CXXThisExpr is an expression without associated declaration.
>
>
> Then which kind of thing should I sent to `CurComponents` if we do not have 
> associated declaration for CXXThisExpr? We can't just send nullptr, right?


Check how compiler handles mapping of `this[0:1]`, which is the same as `*this`.




Comment at: clang/lib/Sema/SemaOpenMP.cpp:16007
+if (!RE->IgnoreParenImpCasts()->isLValue() &&
+(MLValue != clang::Expr::isModifiableLvalueResult::MLV_LValueCast)) {
   SemaRef.Diag(ELoc,

cchen wrote:
> ABataev wrote:
> > Why `MLV_LValueCast` must be allowed here?
> I'm doing this since `isLValue` return false for either C style casting CXX 
> style casting and `MLV_LValueCast` can categorize these two  as 
> `MLV_LValueCast`.
Are they the lvalues according to the standard? If not, no need for this 
analysis.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72811



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72811: [WIP][OPENMP5.0] allow lvalue for motion clause

[Chi Chun Chen via Phabricator via cfe-commits]

cchen marked an inline comment as done.
cchen added a comment.

In D72811#1839561 , @ABataev wrote:

> In D72811#1839538 , @cchen wrote:
>
> > In D72811#1837392 , @jdoerfert 
> > wrote:
> >
> > > Thanks for working on this! While you are at it, `*this` is probably one 
> > > of the most important ones to test and support.
> >
> >
> > Can anyone tell me how to get `ValueDecl` from `CXXThisExpr`? I cannot find 
> > any method return decl in 
> > https://clang.llvm.org/doxygen/classclang_1_1CXXThisExpr.html#details.
> >  Thanks!
>
>
> CXXThisExpr is an expression without associated declaration.


Then which kind of thing should I sent to `CurComponents` if we do not have 
associated declaration for CXXThisExpr? We can't just send nullptr, right?




Comment at: clang/lib/Sema/SemaOpenMP.cpp:16007
+if (!RE->IgnoreParenImpCasts()->isLValue() &&
+(MLValue != clang::Expr::isModifiableLvalueResult::MLV_LValueCast)) {
   SemaRef.Diag(ELoc,

ABataev wrote:
> Why `MLV_LValueCast` must be allowed here?
I'm doing this since `isLValue` return false for either C style casting CXX 
style casting and `MLV_LValueCast` can categorize these two  as 
`MLV_LValueCast`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72811



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72811: [WIP][OPENMP5.0] allow lvalue for motion clause

[Chi Chun Chen via Phabricator via cfe-commits]

cchen marked an inline comment as done.
cchen added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:15235
+  bool VisitUnaryOperator(UnaryOperator *UO) {
+DerefCnt++;
+CurComponents.emplace_back(UO, nullptr);

ABataev wrote:
> cchen wrote:
> > cchen wrote:
> > > ABataev wrote:
> > > > Need a check that this is a dereference op. Also, maybe allow using an 
> > > > addr_of operation?
> > > is addr_of operation allowed in lvalue?
> > > 
> > > In this code:
> > > ```
> > > int arr[50];
> > > 
> > > #pragma omp target map()
> > >   {}
> > > ```
> > > We now reject `` since `RE->IgnoreParenImpCasts()->isLValue()` return 
> > > false. (RE is the expr of ``)
> > BTW, `RE->isLValue()` also return false in this case.
> I'm not saying that `` must be allowed, but something like `*()`. Of 
> course, just `` is an rvalue.
Got it, thanks for the clarification!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72811



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72811: [WIP][OPENMP5.0] allow lvalue for motion clause

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D72811#1839538 , @cchen wrote:

> In D72811#1837392 , @jdoerfert wrote:
>
> > Thanks for working on this! While you are at it, `*this` is probably one of 
> > the most important ones to test and support.
>
>
> Can anyone tell me how to get `ValueDecl` from `CXXThisExpr`? I cannot find 
> any method return decl in 
> https://clang.llvm.org/doxygen/classclang_1_1CXXThisExpr.html#details.
>  Thanks!


CXXThisExpr is an expression without associated declaration.




Comment at: clang/lib/Sema/SemaOpenMP.cpp:15235
+  bool VisitUnaryOperator(UnaryOperator *UO) {
+DerefCnt++;
+CurComponents.emplace_back(UO, nullptr);

cchen wrote:
> cchen wrote:
> > ABataev wrote:
> > > Need a check that this is a dereference op. Also, maybe allow using an 
> > > addr_of operation?
> > is addr_of operation allowed in lvalue?
> > 
> > In this code:
> > ```
> > int arr[50];
> > 
> > #pragma omp target map()
> >   {}
> > ```
> > We now reject `` since `RE->IgnoreParenImpCasts()->isLValue()` return 
> > false. (RE is the expr of ``)
> BTW, `RE->isLValue()` also return false in this case.
I'm not saying that `` must be allowed, but something like `*()`. Of 
course, just `` is an rvalue.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72811



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72811: [WIP][OPENMP5.0] allow lvalue for motion clause

[Chi Chun Chen via Phabricator via cfe-commits]

cchen marked an inline comment as done.
cchen added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:15235
+  bool VisitUnaryOperator(UnaryOperator *UO) {
+DerefCnt++;
+CurComponents.emplace_back(UO, nullptr);

cchen wrote:
> ABataev wrote:
> > Need a check that this is a dereference op. Also, maybe allow using an 
> > addr_of operation?
> is addr_of operation allowed in lvalue?
> 
> In this code:
> ```
> int arr[50];
> 
> #pragma omp target map()
>   {}
> ```
> We now reject `` since `RE->IgnoreParenImpCasts()->isLValue()` return 
> false. (RE is the expr of ``)
BTW, `RE->isLValue()` also return false in this case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72811



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72811: [WIP][OPENMP5.0] allow lvalue for motion clause

[Chi Chun Chen via Phabricator via cfe-commits]

cchen marked an inline comment as done.
cchen added a comment.

In D72811#1837392 , @jdoerfert wrote:

> Thanks for working on this! While you are at it, `*this` is probably one of 
> the most important ones to test and support.


Can anyone tell me how to get `ValueDecl` from `CXXThisExpr`? I cannot find any 
method return decl in 
https://clang.llvm.org/doxygen/classclang_1_1CXXThisExpr.html#details.
Thanks!




Comment at: clang/lib/Sema/SemaOpenMP.cpp:15235
+  bool VisitUnaryOperator(UnaryOperator *UO) {
+DerefCnt++;
+CurComponents.emplace_back(UO, nullptr);

ABataev wrote:
> Need a check that this is a dereference op. Also, maybe allow using an 
> addr_of operation?
is addr_of operation allowed in lvalue?

In this code:
```
int arr[50];

#pragma omp target map()
  {}
```
We now reject `` since `RE->IgnoreParenImpCasts()->isLValue()` return 
false. (RE is the expr of ``)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72811



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72811: [WIP][OPENMP5.0] allow lvalue for motion clause

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:7829
 if (EncounteredME) {
-  const auto *FD = dyn_cast(EncounteredME->getMemberDecl());
-  unsigned FieldIndex = FD->getFieldIndex();
-
-  // Update info about the lowest and highest elements for this struct
-  if (!PartialStruct.Base.isValid()) {
-PartialStruct.LowestElem = {FieldIndex, LB};
-PartialStruct.HighestElem = {FieldIndex, LB};
-PartialStruct.Base = BP;
-  } else if (FieldIndex < PartialStruct.LowestElem.first) {
-PartialStruct.LowestElem = {FieldIndex, LB};
-  } else if (FieldIndex > PartialStruct.HighestElem.first) {
-PartialStruct.HighestElem = {FieldIndex, LB};
+  // MemberExpr can also be FunctionDecl after we allow all lvalue
+  if (const auto *FD =

cchen wrote:
> ABataev wrote:
> > Here we should not have any functiondecls. Must be an assert.
> Don't we need to support something like `#pragma omp target 
> map(r.S.foo()[:12])`?
It does not lead to the actual mapping. Better to inform the user about this 
rather than consuming it silently.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:15283
+  explicit LocatorChecker(
+  OMPClauseMappableExprCommon::MappableExprComponentList )
+  : CurComponents(CurComponents), DerefCnt(0) {}

cchen wrote:
> ABataev wrote:
> > Why do you need components list here?
> Since I need to insert the information in `VisitDeclRefExpr`.
You can do this outside of this class if you need to do it. Still. you have 
this `GetDRE()` member function. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72811



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72811: [WIP][OPENMP5.0] allow lvalue for motion clause

[Chi Chun Chen via Phabricator via cfe-commits]

cchen marked 2 inline comments as done.
cchen added inline comments.



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:7829
 if (EncounteredME) {
-  const auto *FD = dyn_cast(EncounteredME->getMemberDecl());
-  unsigned FieldIndex = FD->getFieldIndex();
-
-  // Update info about the lowest and highest elements for this struct
-  if (!PartialStruct.Base.isValid()) {
-PartialStruct.LowestElem = {FieldIndex, LB};
-PartialStruct.HighestElem = {FieldIndex, LB};
-PartialStruct.Base = BP;
-  } else if (FieldIndex < PartialStruct.LowestElem.first) {
-PartialStruct.LowestElem = {FieldIndex, LB};
-  } else if (FieldIndex > PartialStruct.HighestElem.first) {
-PartialStruct.HighestElem = {FieldIndex, LB};
+  // MemberExpr can also be FunctionDecl after we allow all lvalue
+  if (const auto *FD =

ABataev wrote:
> Here we should not have any functiondecls. Must be an assert.
Don't we need to support something like `#pragma omp target 
map(r.S.foo()[:12])`?



Comment at: clang/lib/Sema/SemaOpenMP.cpp:15283
+  explicit LocatorChecker(
+  OMPClauseMappableExprCommon::MappableExprComponentList )
+  : CurComponents(CurComponents), DerefCnt(0) {}

ABataev wrote:
> Why do you need components list here?
Since I need to insert the information in `VisitDeclRefExpr`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72811



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72811: [WIP][OPENMP5.0] allow lvalue for motion clause

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:7711-7713
+isa(Next->getAssociatedExpression()) ||
+isa(Next->getAssociatedExpression()) ||
+isa(Next->getAssociatedExpression())) &&

This must be updated, I assume. Instead need to check that it is an lvalue.



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:7829
 if (EncounteredME) {
-  const auto *FD = dyn_cast(EncounteredME->getMemberDecl());
-  unsigned FieldIndex = FD->getFieldIndex();
-
-  // Update info about the lowest and highest elements for this struct
-  if (!PartialStruct.Base.isValid()) {
-PartialStruct.LowestElem = {FieldIndex, LB};
-PartialStruct.HighestElem = {FieldIndex, LB};
-PartialStruct.Base = BP;
-  } else if (FieldIndex < PartialStruct.LowestElem.first) {
-PartialStruct.LowestElem = {FieldIndex, LB};
-  } else if (FieldIndex > PartialStruct.HighestElem.first) {
-PartialStruct.HighestElem = {FieldIndex, LB};
+  // MemberExpr can also be FunctionDecl after we allow all lvalue
+  if (const auto *FD =

Here we should not have any functiondecls. Must be an assert.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:15212
+DRE = E;
+const auto *VD = dyn_cast(E->getDecl());
+const auto *FD = dyn_cast(E->getDecl());

I don't think anything except for VarDecls must be allowed here.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:15235
+  bool VisitUnaryOperator(UnaryOperator *UO) {
+DerefCnt++;
+CurComponents.emplace_back(UO, nullptr);

Need a check that this is a dereference op. Also, maybe allow using an addr_of 
operation?



Comment at: clang/lib/Sema/SemaOpenMP.cpp:15244-15245
+  }
+  // checkMapClauseExpressionBase has already added the CurComponents, and we
+  // didn't clean it up, so don't add it here
+  bool VisitArraySubscriptExpr(ArraySubscriptExpr *AE) {

Why you have all these comments here?



Comment at: clang/lib/Sema/SemaOpenMP.cpp:15267-15271
+Expr *LE = BO->getLHS()->IgnoreParenImpCasts();
+Expr *RE = BO->getRHS()->IgnoreParenImpCasts();
+if (LE && Visit(LE) && RE && Visit(RE)) {
+  return true;
+}

Some extra checks for the allowed operators are required.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:15276
+for (Stmt *Child : S->children()) {
+  if (Child && Visit(Child))
+return true;

I would add a check that the child is and expression and not a glvalue, then do 
not analyze it.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:15281
+  }
+  DeclRefExpr *GetDRE() const { return DRE; }
+  explicit LocatorChecker(

`getFoundBase` or something like this?



Comment at: clang/lib/Sema/SemaOpenMP.cpp:15283
+  explicit LocatorChecker(
+  OMPClauseMappableExprCommon::MappableExprComponentList )
+  : CurComponents(CurComponents), DerefCnt(0) {}

Why do you need components list here?



Comment at: clang/lib/Sema/SemaOpenMP.cpp:15347
+  if (!(isa(CurE->getDecl()) ||
+isa(CurE->getDecl(
 return nullptr;

FunctionDecls should not be allowed here



Comment at: clang/lib/Sema/SemaOpenMP.cpp:15369
+  if (!isa(CurE->getMemberDecl()) &&
+  !isa(CurE->getMemberDecl())) {
 if (!NoDiagnose) {

Same here, no function decls



Comment at: clang/lib/Sema/SemaOpenMP.cpp:15535
 } else {
+  if (OrigExpr->IgnoreParenImpCasts()->isLValue()) {
+LocatorChecker Checker(CurComponents);

Why are using `IgnoreParenImpCasts()` here? It drops implicit casts to RValue.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:16007
+if (!RE->IgnoreParenImpCasts()->isLValue() &&
+(MLValue != clang::Expr::isModifiableLvalueResult::MLV_LValueCast)) {
   SemaRef.Diag(ELoc,

Why `MLV_LValueCast` must be allowed here?



Comment at: clang/lib/Sema/SemaOpenMP.cpp:16009
   SemaRef.Diag(ELoc,
diag::err_omp_expected_named_var_member_or_array_expression)
   << RE->getSourceRange();

Message should be updated too.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:16057
 const auto *FD = dyn_cast(CurDeclaration);
+const auto *FuncD = dyn_cast(CurDeclaration);
 

Again, functiondecls must not be allowed here. I would suggest emitting a 
warning for them (that user mapped a function) a drop them from the mapping 
list.



Comment at: clang/test/OpenMP/target_teams_map_messages.cpp:437
 #pragma omp target data map(to x) 

[PATCH] D72811: [WIP][OPENMP5.0] allow lvalue for motion clause

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

Thanks for working on this! While you are at it, `*this` is probably one of the 
most important ones to test and support.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72811



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72811: [WIP][OPENMP5.0] allow lvalue for motion clause

[Chi Chun Chen via Phabricator via cfe-commits]

cchen updated this revision to Diff 240029.
cchen added a comment.

Add a checker for locator (test is not complete yet).

The logic of finding basic decl in LocatorChecker:

- `int *B; omp target map(*B)`
  - B
- pointer count (* in type): 1
- deref count: 1
- B is basic decl since pointer count equal to deref count
- `int *B, l; omp target map(*(B+l))`
  - B
- pointer count (* in type): 1
- deref count: 1
- B is basic decl since pointer count equal to deref count
  - l
- pointer count (* in type): 0
- deref count: 1
- pointer count = 0, deref count 1 => not equal
- `int **B, **l; omp target map((B+**l)[1][2]`
  - B
- pointer count (* in type): 2
- deref count: 2 (2 array subscript)
- B is basic decl since pointer count equal to deref count
  - l
- pointer count (* in type): 2
- deref count: 4 (2 array subscript, 2 unary op)
- pointer count = 2, deref count 4 => not equal


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72811

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
@@ -94,7 +94,7 @@
 #pragma omp target update to(S2::S2sc)
 #pragma omp target update to(to)
 #pragma omp target update to(y x) // expected-error {{expected ',' or ')' in 'to' clause}}
-#pragma omp target update to(argc > 0 ? x : y) // expected-error 2 {{expected expression containing only member accesses and/or array sections based on named variables}} 
+#pragma omp target update to(argc > 0 ? x : y)
 #pragma omp target update to(S1) // expected-error {{'S1' does not refer to a value}}} expected-error {{expected at least one 'to' clause or 'from' clause specified to '#pragma omp target update'}}
 #pragma omp target update to(a, b, c, d, f) // expected-error {{incomplete type 'S1' where a complete type is required}}
 #pragma omp target update to(ba)
@@ -106,7 +106,7 @@
 
 #pragma omp target update to(x, a[:2]) // expected-error {{subscripted value is not an array or pointer}}
 #pragma omp target update to(x, c[:]) // expected-error {{subscripted value is not an array or pointer}}
-#pragma omp target update to(x, (m+1)[2]) // expected-error 2 {{expected expression containing only member accesses and/or array sections based on named variables}}
+#pragma omp target update to(x, (m+1)[2])
 #pragma omp target update to(s7.i, s7.a[:3])
 #pragma omp target update to(s7.s6[1].aa[0:5])
 #pragma omp target update to(x, s7.s6[:5].aa[6]) // expected-error {{OpenMP array section is not allowed here}}
@@ -147,7 +147,7 @@
 #pragma omp target update to(S2::S2sc)
 #pragma omp target update to(to)
 #pragma omp target update to(y x) // expected-error {{expected ',' or ')' in 'to' clause}}
-#pragma omp target update to(argc > 0 ? x : y) // expected-error {{expected expression containing only member accesses and/or array sections based on named variables}} expected-error {{expected at least one 'to' clause or 'from' clause specified to '#pragma omp target update'}}
+#pragma omp target update to(argc > 0 ? x : y)
 #pragma omp target update to(S1) // expected-error {{'S1' does not refer to a value}}} expected-error {{expected at least one 'to' clause or 'from' clause specified to '#pragma omp target update'}}
 #pragma omp target update to(a, b, c, d, f) // expected-error {{incomplete type 'S1' where a complete type is required}}
 #pragma omp target update to(ba)
@@ -159,7 +159,7 @@
 
 #pragma omp target update to(x, a[:2]) // expected-error {{subscripted value is not an array or pointer}}
 #pragma omp target update to(x, c[:]) // expected-error {{subscripted value is not an array or pointer}}
-#pragma omp target update to(x, (m+1)[2]) // expected-error {{expected expression containing only member accesses and/or array sections based on named variables}}
+#pragma omp target update to(x, (m+1)[2])
 #pragma omp target update to(s7.i, s7.a[:3])
 #pragma omp target update to(s7.s6[1].aa[0:5])
 #pragma omp target update to(x, 

[PATCH] D72811: [WIP][OPENMP5.0] allow lvalue for motion clause

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:15442-15443
+  CurComponents.emplace_back(CurE, nullptr);
+} else if (auto *CurE = dyn_cast(E)) {
+  E = CurE->getLHS()->IgnoreParenImpCasts();
 } else {

cchen wrote:
> cchen wrote:
> > ABataev wrote:
> > > cchen wrote:
> > > > ABataev wrote:
> > > > > cchen wrote:
> > > > > > cchen wrote:
> > > > > > > ABataev wrote:
> > > > > > > > Why just the LHS is analyzed? Also, what about support for 
> > > > > > > > other expressions, like casting, call, choose etc., which may 
> > > > > > > > result in lvalue?
> > > > > > > 1. LHS: I'll fix that
> > > > > > > 2. I'll add support for casting, call, etc
> > > > > > > 3. For "choose" are you referring to something like (a < b ? b : 
> > > > > > > a)?
> > > > > > For the handling of BinaryOperator, I'm not sure why should we 
> > > > > > handle RHS since the possible source code I can imagine is 
> > > > > > `*(ptr+l)` or something like `(ptr+l)[3]`.
> > > > > `*(2+ptr)` is correct too. And, btw, `3[ptr+1]` too, especially in C. 
> > > > > Moreover, something like `*(b+c)` is also allowed. That's why I said 
> > > > > that better to avoid deep analysis of lvalues, there are too many 
> > > > > combinations and better to switch to something basic.
> > > > But at least we need the base declaration for motion/map clause, right? 
> > > > And to achieve this, we need to peel the expression to get the 
> > > > DeclRefExpr.
> > > What's the base declaration in `*(a+b)`?
> > I understand your point, I'm just not sure how to handle (do not analyze) 
> > this given that we need to fill VarDecl for OMPToClause/OMPFromClause.
> But is *(a+b) even valid? Clang and GCC both emit error message: 
> https://godbolt.org/z/jqDBx_
Yes, in this form it is not. But here is another form `*(a+*b)` or 
`*(a+(unsigned long long)b)`. You an try to filter rvalues out of all these 
forms, though and still try to find a base decl. But still, there are going to 
be some cases where you won't be possible to handle them correctly, like 
`foo()`.
If you still want to dig that deep, I would recommend to use 
RecursiseStmtVisitor. If an expression is rvalue - throw it out of analysis. If 
base decl is not single, not variable and not member decl - emit an error.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72811



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72811: [WIP][OPENMP5.0] allow lvalue for motion clause

[Chi Chun Chen via Phabricator via cfe-commits]

cchen marked an inline comment as done.
cchen added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:15442-15443
+  CurComponents.emplace_back(CurE, nullptr);
+} else if (auto *CurE = dyn_cast(E)) {
+  E = CurE->getLHS()->IgnoreParenImpCasts();
 } else {

cchen wrote:
> ABataev wrote:
> > cchen wrote:
> > > ABataev wrote:
> > > > cchen wrote:
> > > > > cchen wrote:
> > > > > > ABataev wrote:
> > > > > > > Why just the LHS is analyzed? Also, what about support for other 
> > > > > > > expressions, like casting, call, choose etc., which may result in 
> > > > > > > lvalue?
> > > > > > 1. LHS: I'll fix that
> > > > > > 2. I'll add support for casting, call, etc
> > > > > > 3. For "choose" are you referring to something like (a < b ? b : a)?
> > > > > For the handling of BinaryOperator, I'm not sure why should we handle 
> > > > > RHS since the possible source code I can imagine is `*(ptr+l)` or 
> > > > > something like `(ptr+l)[3]`.
> > > > `*(2+ptr)` is correct too. And, btw, `3[ptr+1]` too, especially in C. 
> > > > Moreover, something like `*(b+c)` is also allowed. That's why I said 
> > > > that better to avoid deep analysis of lvalues, there are too many 
> > > > combinations and better to switch to something basic.
> > > But at least we need the base declaration for motion/map clause, right? 
> > > And to achieve this, we need to peel the expression to get the 
> > > DeclRefExpr.
> > What's the base declaration in `*(a+b)`?
> I understand your point, I'm just not sure how to handle (do not analyze) 
> this given that we need to fill VarDecl for OMPToClause/OMPFromClause.
But is *(a+b) even valid? Clang and GCC both emit error message: 
https://godbolt.org/z/jqDBx_


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72811



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72811: [WIP][OPENMP5.0] allow lvalue for motion clause

[Chi Chun Chen via Phabricator via cfe-commits]

cchen marked an inline comment as done.
cchen added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:15442-15443
+  CurComponents.emplace_back(CurE, nullptr);
+} else if (auto *CurE = dyn_cast(E)) {
+  E = CurE->getLHS()->IgnoreParenImpCasts();
 } else {

ABataev wrote:
> cchen wrote:
> > ABataev wrote:
> > > cchen wrote:
> > > > cchen wrote:
> > > > > ABataev wrote:
> > > > > > Why just the LHS is analyzed? Also, what about support for other 
> > > > > > expressions, like casting, call, choose etc., which may result in 
> > > > > > lvalue?
> > > > > 1. LHS: I'll fix that
> > > > > 2. I'll add support for casting, call, etc
> > > > > 3. For "choose" are you referring to something like (a < b ? b : a)?
> > > > For the handling of BinaryOperator, I'm not sure why should we handle 
> > > > RHS since the possible source code I can imagine is `*(ptr+l)` or 
> > > > something like `(ptr+l)[3]`.
> > > `*(2+ptr)` is correct too. And, btw, `3[ptr+1]` too, especially in C. 
> > > Moreover, something like `*(b+c)` is also allowed. That's why I said that 
> > > better to avoid deep analysis of lvalues, there are too many combinations 
> > > and better to switch to something basic.
> > But at least we need the base declaration for motion/map clause, right? And 
> > to achieve this, we need to peel the expression to get the DeclRefExpr.
> What's the base declaration in `*(a+b)`?
I understand your point, I'm just not sure how to handle (do not analyze) this 
given that we need to fill VarDecl for OMPToClause/OMPFromClause.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72811



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72811: [WIP][OPENMP5.0] allow lvalue for motion clause

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:15442-15443
+  CurComponents.emplace_back(CurE, nullptr);
+} else if (auto *CurE = dyn_cast(E)) {
+  E = CurE->getLHS()->IgnoreParenImpCasts();
 } else {

cchen wrote:
> ABataev wrote:
> > cchen wrote:
> > > cchen wrote:
> > > > ABataev wrote:
> > > > > Why just the LHS is analyzed? Also, what about support for other 
> > > > > expressions, like casting, call, choose etc., which may result in 
> > > > > lvalue?
> > > > 1. LHS: I'll fix that
> > > > 2. I'll add support for casting, call, etc
> > > > 3. For "choose" are you referring to something like (a < b ? b : a)?
> > > For the handling of BinaryOperator, I'm not sure why should we handle RHS 
> > > since the possible source code I can imagine is `*(ptr+l)` or something 
> > > like `(ptr+l)[3]`.
> > `*(2+ptr)` is correct too. And, btw, `3[ptr+1]` too, especially in C. 
> > Moreover, something like `*(b+c)` is also allowed. That's why I said that 
> > better to avoid deep analysis of lvalues, there are too many combinations 
> > and better to switch to something basic.
> But at least we need the base declaration for motion/map clause, right? And 
> to achieve this, we need to peel the expression to get the DeclRefExpr.
What's the base declaration in `*(a+b)`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72811



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72811: [WIP][OPENMP5.0] allow lvalue for motion clause

[Chi Chun Chen via Phabricator via cfe-commits]

cchen marked an inline comment as done.
cchen added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:15442-15443
+  CurComponents.emplace_back(CurE, nullptr);
+} else if (auto *CurE = dyn_cast(E)) {
+  E = CurE->getLHS()->IgnoreParenImpCasts();
 } else {

ABataev wrote:
> cchen wrote:
> > cchen wrote:
> > > ABataev wrote:
> > > > Why just the LHS is analyzed? Also, what about support for other 
> > > > expressions, like casting, call, choose etc., which may result in 
> > > > lvalue?
> > > 1. LHS: I'll fix that
> > > 2. I'll add support for casting, call, etc
> > > 3. For "choose" are you referring to something like (a < b ? b : a)?
> > For the handling of BinaryOperator, I'm not sure why should we handle RHS 
> > since the possible source code I can imagine is `*(ptr+l)` or something 
> > like `(ptr+l)[3]`.
> `*(2+ptr)` is correct too. And, btw, `3[ptr+1]` too, especially in C. 
> Moreover, something like `*(b+c)` is also allowed. That's why I said that 
> better to avoid deep analysis of lvalues, there are too many combinations and 
> better to switch to something basic.
But at least we need the base declaration for motion/map clause, right? And to 
achieve this, we need to peel the expression to get the DeclRefExpr.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72811



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72811: [WIP][OPENMP5.0] allow lvalue for motion clause

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:15442-15443
+  CurComponents.emplace_back(CurE, nullptr);
+} else if (auto *CurE = dyn_cast(E)) {
+  E = CurE->getLHS()->IgnoreParenImpCasts();
 } else {

cchen wrote:
> cchen wrote:
> > ABataev wrote:
> > > Why just the LHS is analyzed? Also, what about support for other 
> > > expressions, like casting, call, choose etc., which may result in lvalue?
> > 1. LHS: I'll fix that
> > 2. I'll add support for casting, call, etc
> > 3. For "choose" are you referring to something like (a < b ? b : a)?
> For the handling of BinaryOperator, I'm not sure why should we handle RHS 
> since the possible source code I can imagine is `*(ptr+l)` or something like 
> `(ptr+l)[3]`.
`*(2+ptr)` is correct too. And, btw, `3[ptr+1]` too, especially in C. Moreover, 
something like `*(b+c)` is also allowed. That's why I said that better to avoid 
deep analysis of lvalues, there are too many combinations and better to switch 
to something basic.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72811



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72811: [WIP][OPENMP5.0] allow lvalue for motion clause

[Chi Chun Chen via Phabricator via cfe-commits]

cchen marked an inline comment as done.
cchen added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:15442-15443
+  CurComponents.emplace_back(CurE, nullptr);
+} else if (auto *CurE = dyn_cast(E)) {
+  E = CurE->getLHS()->IgnoreParenImpCasts();
 } else {

cchen wrote:
> ABataev wrote:
> > Why just the LHS is analyzed? Also, what about support for other 
> > expressions, like casting, call, choose etc., which may result in lvalue?
> 1. LHS: I'll fix that
> 2. I'll add support for casting, call, etc
> 3. For "choose" are you referring to something like (a < b ? b : a)?
For the handling of BinaryOperator, I'm not sure why should we handle RHS since 
the possible source code I can imagine is `*(ptr+l)` or something like 
`(ptr+l)[3]`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72811



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72811: [WIP][OPENMP5.0] allow lvalue for motion clause

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

Hmm, we still need to find some basic decl to remap it successfully at the 
codegen. Not sure that we'll be able to support it in full. It would be good to 
investigate how we can handle them at the codegen.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72811



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72811: [WIP][OPENMP5.0] allow lvalue for motion clause

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

Not sure that we need to dig that deep. Maybe just keep thw original analysis 
for the known expressions and map all other lvalues without some extra 
analysis, just like DeclRefExpr nodes?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72811



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72811: [WIP][OPENMP5.0] allow lvalue for motion clause

[Chi Chun Chen via Phabricator via cfe-commits]

cchen marked 2 inline comments as done.
cchen added inline comments.



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:7709-7712
 isa(Next->getAssociatedExpression()) ||
 isa(Next->getAssociatedExpression()) ||
-isa(Next->getAssociatedExpression())) &&
+isa(Next->getAssociatedExpression()) ||
+isa(Next->getAssociatedExpression())) &&

ABataev wrote:
> This assertion must check that the expression is just an lvalue, no?
Yes.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:15442-15443
+  CurComponents.emplace_back(CurE, nullptr);
+} else if (auto *CurE = dyn_cast(E)) {
+  E = CurE->getLHS()->IgnoreParenImpCasts();
 } else {

ABataev wrote:
> Why just the LHS is analyzed? Also, what about support for other expressions, 
> like casting, call, choose etc., which may result in lvalue?
1. LHS: I'll fix that
2. I'll add support for casting, call, etc
3. For "choose" are you referring to something like (a < b ? b : a)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72811



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72811: [WIP][OPENMP5.0] allow lvalue for motion clause

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:7709-7712
 isa(Next->getAssociatedExpression()) ||
 isa(Next->getAssociatedExpression()) ||
-isa(Next->getAssociatedExpression())) &&
+isa(Next->getAssociatedExpression()) ||
+isa(Next->getAssociatedExpression())) &&

This assertion must check that the expression is just an lvalue, no?



Comment at: clang/lib/Sema/SemaOpenMP.cpp:15442-15443
+  CurComponents.emplace_back(CurE, nullptr);
+} else if (auto *CurE = dyn_cast(E)) {
+  E = CurE->getLHS()->IgnoreParenImpCasts();
 } else {

Why just the LHS is analyzed? Also, what about support for other expressions, 
like casting, call, choose etc., which may result in lvalue?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72811



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72811: [WIP][OPENMP5.0] allow lvalue for motion clause

[Chi Chun Chen via Phabricator via cfe-commits]

cchen created this revision.
cchen added a reviewer: ABataev.
Herald added subscribers: cfe-commits, guansong.
Herald added a reviewer: jdoerfert.
Herald added a project: clang.

In OpenMP 5.0 we allow:

  int *p;
  
  int **p;
  
  struct S {
int struct *sp;
double *d;
struct S **sp_arr;
  };
  struct S *s;
  
  Now only add test for motion clause, after the implementation is fine,
  will add test for map since the core logic is in the same helper
  function.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D72811

Files:
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/lib/Sema/SemaOpenMP.cpp
  clang/test/OpenMP/target_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
@@ -106,7 +106,7 @@
 
 #pragma omp target update to(x, a[:2]) // expected-error {{subscripted value is not an array or pointer}}
 #pragma omp target update to(x, c[:]) // expected-error {{subscripted value is not an array or pointer}}
-#pragma omp target update to(x, (m+1)[2]) // expected-error 2 {{expected expression containing only member accesses and/or array sections based on named variables}}
+#pragma omp target update to(x, (m+1)[2])
 #pragma omp target update to(s7.i, s7.a[:3])
 #pragma omp target update to(s7.s6[1].aa[0:5])
 #pragma omp target update to(x, s7.s6[:5].aa[6]) // expected-error {{OpenMP array section is not allowed here}}
@@ -159,7 +159,7 @@
 
 #pragma omp target update to(x, a[:2]) // expected-error {{subscripted value is not an array or pointer}}
 #pragma omp target update to(x, c[:]) // expected-error {{subscripted value is not an array or pointer}}
-#pragma omp target update to(x, (m+1)[2]) // expected-error {{expected expression containing only member accesses and/or array sections based on named variables}}
+#pragma omp target update to(x, (m+1)[2])
 #pragma omp target update to(s7.i, s7.a[:3])
 #pragma omp target update to(s7.s6[1].aa[0:5])
 #pragma omp target update to(x, s7.s6[:5].aa[6]) // expected-error {{OpenMP array section is not allowed here}}
Index: clang/test/OpenMP/target_update_from_messages.cpp
===
--- clang/test/OpenMP/target_update_from_messages.cpp
+++ clang/test/OpenMP/target_update_from_messages.cpp
@@ -106,7 +106,7 @@
 
 #pragma omp target update from(x, a[:2]) // expected-error {{subscripted value is not an array or pointer}}
 #pragma omp target update from(x, c[:]) // expected-error {{subscripted value is not an array or pointer}}
-#pragma omp target update from(x, (m+1)[2]) // expected-error 2 {{expected expression containing only member accesses and/or array sections based on named variables}}
+#pragma omp target update from(x, (m+1)[2])
 #pragma omp target update from(s7.i, s7.a[:3])
 #pragma omp target update from(s7.s6[1].aa[0:5])
 #pragma omp target update from(x, s7.s6[:5].aa[6]) // expected-error {{OpenMP array section is not allowed here}}
@@ -160,7 +160,7 @@
 
 #pragma omp target update from(x, a[:2]) // expected-error {{subscripted value is not an array or pointer}}
 #pragma omp target update from(x, c[:]) // expected-error {{subscripted value is not an array or pointer}}
-#pragma omp target update from(x, (m+1)[2]) // expected-error {{expected expression containing only member accesses and/or array sections based on named variables}}
+#pragma omp target update from(x, (m+1)[2])
 #pragma omp target update from(s7.i, s7.a[:3])
 #pragma omp target update from(s7.s6[1].aa[0:5])
 #pragma omp target update from(x, s7.s6[:5].aa[6]) // expected-error {{OpenMP array section is not allowed here}}
Index: clang/test/OpenMP/target_update_codegen.cpp
===
--- clang/test/OpenMP/target_update_codegen.cpp
+++ clang/test/OpenMP/target_update_codegen.cpp
@@ -291,5 +291,405 @@
   #pragma omp target update from(arg) if(arg) device(4)
   {++arg;}
 }
+#endif
+///==///
+// RUN: %clang_cc1 -DCK5 -verify -fopenmp -fopenmp-targets=powerpc64le-ibm-linux-gnu -x c++ -triple powerpc64le-unknown-unknown -emit-llvm %s -o - | FileCheck %s --check-prefix CK5 --check-prefix CK5-64
+// RUN: %clang_cc1 -DCK5 -fopenmp -fopenmp-targets=powerpc64le-ibm-linux-gnu -x c++ -std=c++11 -triple powerpc64le-unknown-unknown -emit-pch -o %t %s
+// RUN: %clang_cc1 -fopenmp -fopenmp-targets=powerpc64le-ibm-linux-gnu -x c++ -triple powerpc64le-unknown-unknown -std=c++11 -include-pch %t -verify %s -emit-llvm -o - | FileCheck %s  --check-prefix CK5 --check-prefix CK5-64
+// RUN: %clang_cc1 -DCK5 -verify -fopenmp