[PATCH] D10732: [OPENMP 4.0] Initial support for array sections.
Hal Finkel
hfinkel at anl.gov
Wed Jul 22 15:02:01 PDT 2015
----- Original Message -----
> From: "Richard Smith" <richard at metafoo.co.uk>
> To: "a bataev" <a.bataev at hotmail.com>, hfinkel at anl.gov, ejstotzer at gmail.com, fraggamuffin at gmail.com
> Cc: cfe-commits at 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
More information about the cfe-commits
mailing list