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

[Samuel Antao via cfe-commits]

sfantao added a comment.

Hi Alexey,

Thanks for the review!


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:7767-7768
@@ -7766,4 +7766,4 @@
   "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_omp_array_section_leads_to_non_contiguous_storage : Error<
+  "employed array section is or can be incompatible with contiguous storage requirements">;
 def err_omp_union_type_not_allowed : Error<
----------------
ABataev wrote:
> If you say 'can be' incompatible, then this must be a warning, not an error
I agree the diagnostic description is not the best. I changed it to: "can't prove employed array section specifies contiguous storage".

This has to be an error because there is no way to express non-contiguous in the offload runtime interface. So, we would have to codegen traps. The user would end up getting runtime crashes instead of a diagnostic that would be far more useful to direct the user to take the right actions.  

================
Comment at: lib/Sema/SemaOpenMP.cpp:8988-8995
@@ +8987,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:
> I can't agree with that. For example:
> ```
> const int n = 0;
> arr[n:]
> ```
> It won't work with your solution, though we shall support it
Hi Alexey,

That example works fine. Note that CheckArrayExpressionReferToWholeSize only results in a error if  `AllowWholeSizeArraySection = false`. It is initially set to true, and will be set to false as components of the expression prove to be incompatible with that. I added a few regression test for when the bounds come from variables.

So:

```
struct S1 {
  int a;
  int b;
}
struct S2 {
  S1 a[10];
  int b;
}

foo (int arg) {
  int a[5][6];
  const int n = 0;
  S2 s;

  // valid - the array expression is in the right most component.
  #pragma omp target map(a[arg:])
  #pragma omp target map(a[:arg])
  #pragma omp target map(a[n:])
  // valid - this is valid only if n is zero and the compiler can prove that.
  #pragma omp target map(a[:][n:])

  // invalid - is only valid if arg is zero and that cannot be proved.
  #pragma omp target map(a[:][arg:])

  // invalid - it is contiguous storage but the OpenMP 4.5 spec explicitly say array sections only allowed in the rightmost expression if struct fields are involved.
  #pragma omp target map(s.a[n:1].b)
}
```

================
Comment at: lib/Sema/SemaOpenMP.cpp:9208-9220
@@ -9113,6 +9207,15 @@
 
+      // Determine the dimension we care about. We need to skip all the nested
+      // array sections to determine that.
+      unsigned Dimension = 0;
+      auto *BaseE = E;
+      while (auto *SE = dyn_cast<OMPArraySectionExpr>(BaseE)) {
+        BaseE = SE->getBase()->IgnoreParenImpCasts();
+        ++Dimension;
+      }
+
       // OpenMP 4.5 [2.15.5.1, map Clause, Restrictions, C++, p.1]
       //  If the type of a list item is a reference to a type T then the type
       //  will be considered to be T for all purposes of this clause.
-      QualType CurType = E->getType();
+      QualType CurType = BaseE->getType();
       if (CurType->isReferenceType())
----------------
ABataev wrote:
> OMPArraySectionExpr has static function getBaseOriginalType()
Oh, great, I hadn't noticed. I am using that now. Thanks!


http://reviews.llvm.org/D17547