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

[Richard Smith via cfe-commits]

rsmith added a comment.

Thanks for the rework, the general approach here seems reasonable.


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:7680
@@ +7679,3 @@
+def err_omp_section_length_undefined : Error<
+  "section length is unspecified, but subscripted value is not an array">;
+
----------------
How about "section length is unspecified and cannot be inferred because subscripted value is not an array"?

Presumably there should also be an error for "section length is unspecified and cannot be inferred because subscripted value is an array of unknown bound"?

================
Comment at: lib/AST/ASTContext.cpp:1046-1047
@@ -1045,1 +1045,4 @@
 
+  // Placeholder type for OMP array sections.
+  InitBuiltinType(OMPArraySectionTy,  BuiltinType::OMPArraySection);
+
----------------
Condition this behind `if (LangOpts.OpenMP)`.

================
Comment at: lib/AST/Stmt.cpp:18
@@ -17,2 +17,3 @@
 #include "clang/AST/ExprObjC.h"
+#include "clang/AST/ExprOpenMP.h"
 #include "clang/AST/Stmt.h"
----------------
This appears to be unused?

================
Comment at: lib/Parse/ParseExpr.cpp:1400
@@ +1399,3 @@
+      ExprResult Idx, Length;
+      bool OMPArraySectionIsFound = false;
+      SourceLocation ColonLoc;
----------------
This appears to be unnecessary; you can use `ColonLoc.isValid()`.

================
Comment at: lib/Parse/ParseExpr.cpp:1406
@@ +1405,3 @@
+      } else {
+        ColonProtectionRAIIObject RAII(*this);
+        // Parse [: or [ expr or [ expr :
----------------
Please condition your new parsing, and in particular this colon protection, on `getLangOpts().OpenMP`, so we can still recover properly from typos like `A[X:foo()]` in C++.

================
Comment at: lib/Sema/SemaExpr.cpp:4001-4004
@@ +4000,6 @@
+  unsigned ArraySectionCount = 0;
+  while (auto *OASE = dyn_cast<OMPArraySectionExpr>(Base)) {
+    Base = OASE->getBase();
+    ++ArraySectionCount;
+  }
+  auto OriginalTy = Base->getType();
----------------
Should you `IgnoreParens()` here?

================
Comment at: lib/Sema/SemaExpr.cpp:4006
@@ +4005,3 @@
+  auto OriginalTy = Base->getType();
+  for (unsigned i = 0; i < ArraySectionCount; ++i) {
+    if (OriginalTy->isAnyPointerType())
----------------
Please either consistently capitalize (per prevailing style in this file), or do not capitalize (per new LLVM coding style), your local variables; don't mix and match.

================
Comment at: lib/Sema/SemaExpr.cpp:4017
@@ +4016,3 @@
+
+ExprResult Sema::ActOnOMPArraySectionExpr(Scope *S, Expr *Base,
+                                          SourceLocation LBLoc,
----------------
I don't think you should need a `Scope` here; you shouldn't be performing any unqualified lookups while handling this expression.

================
Comment at: lib/Sema/SemaExpr.cpp:4022-4026
@@ +4021,7 @@
+                                          SourceLocation RBLoc) {
+  // Handle any non-overload placeholder types in the base and index
+  // expressions.  We can't handle overloads here because the other
+  // operand might be an overloadable type, in which case the overload
+  // resolution for the operator overload should get the first crack
+  // at the overload.
+  if (Base->getType()->isNonOverloadPlaceholderType() &&
----------------
This comment doesn't seem right -- array section expressions aren't overloadable.

================
Comment at: lib/Sema/SemaExpr.cpp:4027
@@ +4026,3 @@
+  // at the overload.
+  if (Base->getType()->isNonOverloadPlaceholderType() &&
+      !Base->getType()->isSpecificPlaceholderType(
----------------
Likewise, these `isNonOverloadablePlaceholderType()` calls don't seem right; I think you should be checking for /any/ placeholder type other than `OMPArraySection` here.

================
Comment at: lib/Sema/SemaExpr.cpp:4072
@@ +4071,3 @@
+  if (LowerBound) {
+    if (!LowerBound->getType()->isIntegerType())
+      return ExprError(Diag(LowerBound->getExprLoc(),
----------------
This should presumably follow the rules for the underlying language. In particular, in C++, we should try to contextually implicitly convert to an integral type -- see `Sema::PerformContextualImplicitConversion`).

================
Comment at: lib/Sema/SemaExpr.cpp:4110
@@ +4109,3 @@
+    llvm::APSInt LowerBoundValue;
+    if (LowerBound->EvaluateAsInt(LowerBoundValue, Context)) {
+      // OpenMP 4.0, [2.4 Array Sections]
----------------
Same comment here as before: using constant folding to drive an error is not OK; you should only error here if the expression is an ICE.

================
Comment at: lib/Sema/SemaExpr.cpp:4112
@@ +4111,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 a semantic rule? (Are we required to reject it, are we permitted to reject it, or is a violation just UB?)

================
Comment at: lib/Sema/SemaExpr.cpp:4146-4154
@@ +4145,11 @@
+  if (!LengthExpr) {
+    if (ColonLoc.isInvalid()) {
+      LengthExpr = ActOnIntegerConstant(SourceLocation(), /*Val=*/1).get();
+    } else {
+      if (auto *CAT =
+              dyn_cast<ConstantArrayType>(OriginalTy->getAsArrayTypeUnsafe())) {
+        llvm::APInt SizeVal = CAT->getSize();
+        LengthExpr = IntegerLiteral::Create(Context, SizeVal,
+                                            Context.getIntPtrType(), RBLoc);
+      } else {
+        auto *VAT = cast<VariableArrayType>(OriginalTy->getAsArrayTypeUnsafe());
----------------
I'm not particularly happy about fabricating an expression here, especially one with an invalid location. Maybe instead store a null `LengthExpr` in this case and provide accessors on your AST node to determine whether it's an implicit 1-element section or an implicit to-the-end section (maybe based on whether `ColonLoc.isValid()`).

================
Comment at: lib/Sema/SemaExpr.cpp:4165-4178
@@ +4164,16 @@
+  }
+  // Build upper bound expression.
+  ExprResult UpperBound = LengthExpr;
+  if (LowerBound)
+    UpperBound = BuildBinOp(S, RBLoc, BO_Add, LengthExpr, LowerBound);
+  if (UpperBound.isInvalid())
+    return ExprError();
+  UpperBound =
+      BuildBinOp(S, RBLoc, BO_Sub, UpperBound.get(),
+                 ActOnIntegerConstant(SourceLocation(), /*Val=*/1).get());
+  if (UpperBound.isInvalid())
+    return ExprError();
+  UpperBound = ActOnFinishFullExpr(UpperBound.get());
+  if (UpperBound.isInvalid())
+    return ExprError();
+
----------------
Why build and store an upper bound expression? It seems that this information can always be reconstructed by the user of an `OMPArraySectionExpr`.


http://reviews.llvm.org/D10732