----- Original Message ----- > From: "Richard Smith" <rich...@metafoo.co.uk> > To: "a bataev" <a.bat...@hotmail.com>, hfin...@anl.gov, ejstot...@gmail.com, > fraggamuf...@gmail.com > Cc: cfe-commits@cs.uiuc.edu > Sent: Wednesday, July 22, 2015 3:24:29 PM > Subject: Re: [PATCH] D10732: [OPENMP 4.0] Initial support for array sections. > > rsmith added a comment. > > This seems like a very strange extension. Can you point me at where > in the OpenMP specification it's defined?
Array sections are defined in: http://www.openmp.org/mp-documents/OpenMP4.0.0.pdf - section 2.4 (starting on page 42). -Hal > > 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 > > > > -- Hal Finkel Assistant Computational Scientist Leadership Computing Facility Argonne National Laboratory _______________________________________________ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits