Page MenuHomePhabricator

[OPENMP 4.0] Initial support for array sections.
ClosedPublic

Authored by ABataev on Jun 25 2015, 5:31 AM.

Details

Summary

Adds parsing/sema analysis/serialization/deserialization for array sections in OpenMP constructs (introduced in OpenMP 4.0).
Currently it is allowed to use array sections only in OpenMP clauses that accepts list of expressions.

Diff Detail

Repository
rL LLVM

Event Timeline

ABataev updated this revision to Diff 28453.Jun 25 2015, 5:31 AM
ABataev retitled this revision from to [OPENMP] Initial support for array sections (OpenMP 4.0)..
ABataev updated this object.
ABataev edited the test plan for this revision. (Show Details)
ABataev added a subscriber: Unknown Object (MLST).
ABataev updated this revision to Diff 28456.Jun 25 2015, 5:54 AM

Added tests for array sections serialization/deserialization/AST printing.

ABataev updated this revision to Diff 29156.Jul 7 2015, 1:08 AM
ABataev retitled this revision from [OPENMP] Initial support for array sections (OpenMP 4.0). to [OPENMP 4.0] Initial support for array sections..

Added array type for ArraySectionExpr and new array size modifier - ArraySection

ABataev updated this revision to Diff 29671.Jul 14 2015, 6:17 AM

Fixed processing of array sections without specified length.

ABataev updated this revision to Diff 29895.Jul 16 2015, 5:52 AM

Improved for future codegen.

rsmith edited edge metadata.Jul 22 2015, 1:24 PM

This seems like a very strange extension. Can you point me at where in the OpenMP specification it's defined?

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?)

include/clang/AST/Expr.h
2146 ↗(On Diff #29895)

Have you considered starting an ExprOpenMP.h for OpenMP expressions?

2184–2185 ↗(On Diff #29895)

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.

include/clang/Basic/DiagnosticSemaKinds.td
4915–4916 ↗(On Diff #29895)

"section %select{...} evaluated to negative value %1"?

4918 ↗(On Diff #29895)

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.)

lib/Parse/ParseExpr.cpp
1406–1409 ↗(On Diff #29895)

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.

lib/Parse/ParseOpenMP.cpp
767 ↗(On Diff #29895)

What's this for?

lib/Parse/RAIIObjectsForParser.h
291–292 ↗(On Diff #29895)

It does not appear to manage ColonIsSacred...

lib/Sema/SemaChecking.cpp
8469–8475 ↗(On Diff #29895)

Can you also check the end of the section here?

lib/Sema/SemaExpr.cpp
510–519 ↗(On Diff #29895)

Should we instead create a ConstantArrayType if the size is a constant expression?

3973–3978 ↗(On Diff #29895)

What should happen here:

int x[5];
int y[10];
(b ? x : y[5:])[3]

? Or here:

int x[5][5];
(+x[2:])[2]
4116–4117 ↗(On Diff #29895)

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.

4119 ↗(On Diff #29895)

Is this a constraint, or is this semantics? (Are these bounds required to be ICEs?)

hfinkel edited edge metadata.Jul 22 2015, 3:02 PM
hfinkel added a subscriber: hfinkel.
  • Original Message -----

From: "Richard Smith" <richard@metafoo.co.uk>
To: "a bataev" <a.bataev@hotmail.com>, hfinkel@anl.gov, ejstotzer@gmail.com, fraggamuffin@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

OK, I think what's specified in the OpenMP spec is a little different from what we have here. In particular, an array section does not appear to be a general-purpose expression, it's just a special syntax that can appear only at the top level in certain OpenMP clauses to describe a section of a (possibly multidimensional) array. As such, it doesn't need a real type, doesn't need to be incorporated into normal expression parsing, and so on.

The most straightforward representation would be to give this expression a placeholder type, and make CheckPlaceholderExpr reject it. Then, in the places where it's valid (some OpenMP clause arguments, and on the left-hand side of array indexing and array sections), check for this special case before calling CheckPlaceholderExpr on the base. That's how we deal with other special-case constructs that can only appear in a few places (bound member functions, overloaded function names, and the like). You can then parse it anywhere (when OpenMP is enabled) and it'll get automatically rejected if it appears in a context where it's not permitted.

Please also rename it to OMPArraySectionExpr or similar, since this is not a general-purpose construct and is unlikely to be useful anywhere outside OpenMP.

Richard, thanks for the review!

include/clang/AST/Expr.h
2146 ↗(On Diff #29895)

Ok, moved it to ExprOpenMP.h

include/clang/Basic/DiagnosticSemaKinds.td
4915–4916 ↗(On Diff #29895)

Fixed

4918 ↗(On Diff #29895)

Fixed

lib/Sema/SemaChecking.cpp
8469–8475 ↗(On Diff #29895)

Ok, added

lib/Sema/SemaExpr.cpp
3973–3978 ↗(On Diff #29895)

Here we should get error messages. Reworked it to allow array sections only in base part.

4119 ↗(On Diff #29895)

Bounds are not required to be ICEs. But if they are ICEs we must check that all these values are non-negative

ABataev updated this revision to Diff 30471.Jul 23 2015, 4:25 AM
ABataev edited edge metadata.

Update after review

Richard, any comments?

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

include/clang/Basic/DiagnosticSemaKinds.td
7680 ↗(On Diff #30471)

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"?

lib/AST/ASTContext.cpp
1046–1047 ↗(On Diff #30471)

Condition this behind if (LangOpts.OpenMP).

lib/AST/Stmt.cpp
18 ↗(On Diff #30471)

This appears to be unused?

lib/Parse/ParseExpr.cpp
1400 ↗(On Diff #30471)

This appears to be unnecessary; you can use ColonLoc.isValid().

1406 ↗(On Diff #30471)

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++.

lib/Sema/SemaExpr.cpp
4001–4004 ↗(On Diff #30471)

Should you IgnoreParens() here?

4006 ↗(On Diff #30471)

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.

4017 ↗(On Diff #30471)

I don't think you should need a Scope here; you shouldn't be performing any unqualified lookups while handling this expression.

4022–4026 ↗(On Diff #30471)

This comment doesn't seem right -- array section expressions aren't overloadable.

4027 ↗(On Diff #30471)

Likewise, these isNonOverloadablePlaceholderType() calls don't seem right; I think you should be checking for /any/ placeholder type other than OMPArraySection here.

4072 ↗(On Diff #30471)

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).

4110 ↗(On Diff #30471)

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.

4112 ↗(On Diff #30471)

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?)

4146–4154 ↗(On Diff #30471)

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()).

4165–4178 ↗(On Diff #30471)

Why build and store an upper bound expression? It seems that this information can always be reconstructed by the user of an OMPArraySectionExpr.

ABataev marked 15 inline comments as done.Aug 24 2015, 10:45 AM

Richard, thanks for the review!

include/clang/Basic/DiagnosticSemaKinds.td
7680 ↗(On Diff #30471)

Done.

lib/AST/ASTContext.cpp
1046–1047 ↗(On Diff #30471)

Done, thanks

lib/AST/Stmt.cpp
18 ↗(On Diff #30471)

No, this one is required for getStmtInfoTableEntry(), check_implementations() and others.

lib/Sema/SemaExpr.cpp
4001–4004 ↗(On Diff #30471)

Yes, added.

4006 ↗(On Diff #30471)

Fixed, thanks.

4017 ↗(On Diff #30471)

Removed

4072 ↗(On Diff #30471)

Done.

4110 ↗(On Diff #30471)

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.

4112 ↗(On Diff #30471)

We are permitted to reject it.

4146–4154 ↗(On Diff #30471)

Ok, I'll remove all implicit expressions here and below and will build them in codegen.

ABataev updated this revision to Diff 32974.Aug 24 2015, 10:46 AM
ABataev marked 10 inline comments as done.

Update after review

rsmith accepted this revision.Aug 24 2015, 3:12 PM
rsmith edited edge metadata.

Thanks, LGTM!

lib/Sema/SemaExpr.cpp
4035 ↗(On Diff #32974)

I don't think you need to care whether Base is value-dependent here, because you don't care whether you can evaluate it.

4088 ↗(On Diff #32974)

Functions -> functions

This revision is now accepted and ready to land.Aug 24 2015, 3:12 PM
This revision was automatically updated to reflect the committed changes.
ABataev marked an inline comment as done.