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

[Alexey Bataev via cfe-commits]

ABataev marked 15 inline comments as done.
ABataev added a comment.

Richard, thanks for the review!


================
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">;
+
----------------
rsmith wrote:
> 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"?
Done.

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

================
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"
----------------
rsmith wrote:
> This appears to be unused?
No, this one is required for getStmtInfoTableEntry(), check_implementations() and others.

================
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();
----------------
rsmith wrote:
> Should you `IgnoreParens()` here?
Yes, added.

================
Comment at: lib/Sema/SemaExpr.cpp:4006
@@ +4005,3 @@
+  auto OriginalTy = Base->getType();
+  for (unsigned i = 0; i < ArraySectionCount; ++i) {
+    if (OriginalTy->isAnyPointerType())
----------------
rsmith wrote:
> 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.
Fixed, thanks.

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

================
Comment at: lib/Sema/SemaExpr.cpp:4072
@@ +4071,3 @@
+  if (LowerBound) {
+    if (!LowerBound->getType()->isIntegerType())
+      return ExprError(Diag(LowerBound->getExprLoc(),
----------------
rsmith wrote:
> 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`).
Done.

================
Comment at: lib/Sema/SemaExpr.cpp:4110
@@ +4109,3 @@
+    llvm::APSInt LowerBoundValue;
+    if (LowerBound->EvaluateAsInt(LowerBoundValue, Context)) {
+      // OpenMP 4.0, [2.4 Array Sections]
----------------
rsmith wrote:
> 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.
Well, that is exactly what I'm doing here. I'm checking that this an ICE and only if LowerBound is ICE I'm checking its constraints. EvaluateAsInt() does not emit any errors if LowerBound is not 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()) {
----------------
rsmith wrote:
> 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?)
We are permitted to reject it.

================
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());
----------------
rsmith wrote:
> 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()`).
Ok, I'll remove all implicit expressions here and below and will build them in codegen.


http://reviews.llvm.org/D10732