[PATCH] D10732: [OPENMP 4.0] Initial support for array sections.

[Richard Smith]

rsmith added a comment.

This seems like a very strange extension. Can you point me at where in the OpenMP specification it's defined?

The implementation here seems to be half treating it as producing a single value (like an implicit parameter pack) and half treating it as producing an array. Which is the intended model? What is the type of 'x[5:]' supposed to be? (Same as 'x[5]' or same as 'x' but with a smaller bound?)


================
Comment at: include/clang/AST/Expr.h:2146
@@ -2145,1 +2145,3 @@
 
+/// \brief OpenMP 4.0 [2.4, Array Sections].
+/// To specify an array section in an OpenMP construct, array subscript
----------------
Have you considered starting an ExprOpenMP.h for OpenMP expressions?

================
Comment at: include/clang/AST/Expr.h:2184-2185
@@ +2183,4 @@
+            Base->isTypeDependent() ||
+                (LowerBound && LowerBound->isTypeDependent()) ||
+                (Length && Length->isTypeDependent()),
+            Base->isValueDependent() ||
----------------
What's the type of an array section? Given 'int n[8]', is 'n[:4]' an 'int[4]'? If so, the section is also type-dependent whenever the lower bound or length is value-dependent.

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:4915-4916
@@ +4914,4 @@
+  "section of pointer to incomplete type %0">;
+def err_section_negative : Error<
+  "section %select{lower bound|length}0 is evaluated to a negative value">;
+def err_section_length_undefined : Error<
----------------
"section %select{...} evaluated to negative value %1"?

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:4918
@@ +4917,3 @@
+def err_section_length_undefined : Error<
+  "section length is undefined, but subscripted value is not a sized array">;
+
----------------
undefined -> unspecified

sized array -> array

(Can you take a section on a VLA? There doesn't seem to be any obvious reason why not, since presumably you could construct the VLA type [old_bound - start] as the result.)

================
Comment at: lib/Parse/ParseExpr.cpp:1406-1409
@@ +1405,6 @@
+      } else {
+        bool MayArraySection = ArraySectionAllowed;
+        ArraySectionExpressionRAIIObject RAII(*this, /*Value=*/false);
+        // Parse [: or [ expr or [ expr :
+        if (!MayArraySection || !Tok.is(tok::colon)) {
+          // [ expr
----------------
What are you trying to achieve here? It looks like this bans

  a[ : *a[4:] ]

... which seems like it should be allowed, given that `a[4:]` is supposed to still have array type.

================
Comment at: lib/Parse/ParseOpenMP.cpp:767
@@ -766,2 +766,3 @@
                      Tok.isNot(tok::annot_pragma_openmp_end))) {
-    ColonProtectionRAIIObject ColonRAII(*this, MayHaveTail);
+    ArraySectionExpressionRAIIObject RAII(*this, Kind == OMPC_depend);
+    ColonProtectionRAIIObject ColonRAII(*this,
----------------
What's this for?

================
Comment at: lib/Parse/RAIIObjectsForParser.h:291-292
@@ +290,4 @@
+  /// \brief This sets the Parser::ArraySectionAllowed bool and
+  /// restores it when destroyed. It it also manages Parser::ColonIsSacred for
+  /// correct parsing of array sections.
+  class ArraySectionExpressionRAIIObject {
----------------
It does not appear to manage `ColonIsSacred`...

================
Comment at: lib/Sema/SemaChecking.cpp:8469-8475
@@ -8464,2 +8468,9 @@
       }
+      case Stmt::ArraySectionExprClass: {
+        const ArraySectionExpr *ASE = cast<ArraySectionExpr>(expr);
+        if (ASE->getLowerBound())
+          CheckArrayAccess(ASE->getBase(), ASE->getLowerBound(),
+                           /**ASE=*/nullptr, AllowOnePastEnd > 0);
+        return;
+      }
       case Stmt::UnaryOperatorClass: {
----------------
Can you also check the end of the section here?

================
Comment at: lib/Sema/SemaExpr.cpp:510-519
@@ +509,12 @@
+                                       SourceRange Brackets) {
+  if (isArraySectionType(OriginalTy)) {
+    auto *VAT = C.getAsVariableArrayType(OriginalTy);
+    return C.getVariableArrayType(
+        createArraySectionType(C, VAT->getElementType(), ResultTy, Size,
+                               Brackets),
+        VAT->getSizeExpr(), VAT->getSizeModifier(),
+        VAT->getIndexTypeCVRQualifiers(), VAT->getBracketsRange());
+  }
+  return C.getVariableArrayType(ResultTy, Size, ArrayType::ArraySection,
+                                /*IndexTypeQuals=*/0, Brackets);
+}
----------------
Should we instead create a ConstantArrayType if the size is a constant expression?

================
Comment at: lib/Sema/SemaExpr.cpp:3973-3978
@@ -3945,1 +3972,8 @@
                               Expr *idx, SourceLocation rbLoc) {
+  if (isArraySectionType(base->IgnoreImpCasts()->getType()))
+    return ActOnArraySectionExpr(S, base, lbLoc, idx, SourceLocation(),
+                                 /*Length=*/nullptr, rbLoc);
+  else if (isArraySectionType(idx->IgnoreImpCasts()->getType()))
+    return ActOnArraySectionExpr(S, idx, lbLoc, base, SourceLocation(),
+                                 /*Length=*/nullptr, rbLoc);
+
----------------
What should happen here:

  int x[5];
  int y[10];
  (b ? x : y[5:])[3]

? Or here:

  int x[5][5];
  (+x[2:])[2]

================
Comment at: lib/Sema/SemaExpr.cpp:4116-4117
@@ +4115,4 @@
+    llvm::APSInt LowerBoundValue;
+    if (LowerBound->EvaluateAsInt(LowerBoundValue, Context,
+                                  Expr::SE_AllowSideEffects)) {
+      // OpenMP 4.0, [2.4 Array Sections]
----------------
Whether we accept a program should not depend on the foibles of our constant folding -- you should only reject if the value is an ICE. Warning in the non-ICE case is OK, though.

================
Comment at: lib/Sema/SemaExpr.cpp:4119
@@ +4118,3 @@
+      // OpenMP 4.0, [2.4 Array Sections]
+      // The lower-bound and length must evaluate to non-negative integers.
+      if (LowerBoundValue.isNegative()) {
----------------
Is this a constraint, or is this semantics? (Are these bounds required to be ICEs?)


http://reviews.llvm.org/D10732