[prev in list] [next in list] [prev in thread] [next in thread] 

List:       cfe-commits
Subject:    Re: [PATCH] D10732: [OPENMP 4.0] Initial support for array sections.
From:       Alexey Bataev via cfe-commits <cfe-commits () lists ! llvm ! org>
Date:       2015-08-24 17:45:30
Message-ID: 32dd8d4305247ccd72df0319d67e44e3 () localhost ! localdomain
[Download RAW message or body]

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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[prev in list] [next in list] [prev in thread] [next in thread]