[PATCH] D17547: [OpenMP] Add support for multidimensional array sections in map clause SEMA.

[Samuel Antao via cfe-commits]

sfantao added inline comments.

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:7787-7789
@@ -7786,5 +7786,5 @@
   "bit fields cannot be used to specify storage in a map clause">;
-def err_omp_array_section_in_rightmost_expression : Error<
-  "array section can only be associated with the rightmost variable in a map clause expression">;
+def err_cannot_prove_omp_array_section_specifies_contiguous_storage : Error<
+  "can't prove employed array section specifies contiguous storage">;
 def err_omp_union_type_not_allowed : Error<
   "mapped storage cannot be derived from a union">;
----------------
ABataev wrote:
> 'can't prove' again is not good for an error. Still think this must be a warning or you don't need to diagnose anything if you can't make a decision. In this case, you must consider specified array section as a contiguous.
Now I am only emitting an error for the cases that can be proved wrong. For those that are inconclusive I don't issue any warning. The message also changed to reflect that.

================
Comment at: lib/Sema/SemaOpenMP.cpp:9016-9023
@@ +9015,10 @@
+
+  // If this is an array subscript, it refers to the whole size if the size of
+  // the dimension is constant and equals 1. Also, an array section assumes the
+  // format of an array subscript if no colon is used.
+  if (isa<ArraySubscriptExpr>(E) || (OASE && OASE->getColonLoc().isInvalid())) {
+    if (auto *ATy = dyn_cast<ConstantArrayType>(BaseQTy.getTypePtr()))
+      return ATy->getSize().getSExtValue() == 1;
+    return false;
+  }
+
----------------
ABataev wrote:
> ABataev wrote:
> > I agree with all your examples except for this one
> > ```
> > #pragma omp target map(a[:][arg:])
> > ```
> > For me, this is valid if you can't prove it is non-contiguous. You should consider this as a possibly contiguous. If it is not contiguous, it is user's problem. But if he needs to use such form of expression, I don't see why we should not allow him to do this.
> I don't think this code
> ```
> const int n = 0;
> arr[n:]
> ```
> will work in C. Try to add a test for C, not C++. In C 'const' does not mean you can evaluate the value of the variable during compilation.
In the new diff I am allowing this given that the ranges check is inconclusive. At runtime the behavior will be the same as  map(a[:]), so the runtime map will contain the holes. This falls into those cases that is users' fault.

================
Comment at: lib/Sema/SemaOpenMP.cpp:9016-9023
@@ +9015,10 @@
+
+  // If this is an array subscript, it refers to the whole size if the size of
+  // the dimension is constant and equals 1. Also, an array section assumes the
+  // format of an array subscript if no colon is used.
+  if (isa<ArraySubscriptExpr>(E) || (OASE && OASE->getColonLoc().isInvalid())) {
+    if (auto *ATy = dyn_cast<ConstantArrayType>(BaseQTy.getTypePtr()))
+      return ATy->getSize().getSExtValue() == 1;
+    return false;
+  }
+
----------------
sfantao wrote:
> ABataev wrote:
> > ABataev wrote:
> > > I agree with all your examples except for this one
> > > ```
> > > #pragma omp target map(a[:][arg:])
> > > ```
> > > For me, this is valid if you can't prove it is non-contiguous. You should consider this as a possibly contiguous. If it is not contiguous, it is user's problem. But if he needs to use such form of expression, I don't see why we should not allow him to do this.
> > I don't think this code
> > ```
> > const int n = 0;
> > arr[n:]
> > ```
> > will work in C. Try to add a test for C, not C++. In C 'const' does not mean you can evaluate the value of the variable during compilation.
> In the new diff I am allowing this given that the ranges check is inconclusive. At runtime the behavior will be the same as  map(a[:]), so the runtime map will contain the holes. This falls into those cases that is users' fault.
So if you have:
```
const int n = 0;
arr[:][n:]
```

This is not a probably anymore because the check in the second dimension will be inconclusive, so it will be allowed even if n can't be proved to be zero.

I'm adding a test anyways.


http://reviews.llvm.org/D17547