This is an archive of the discontinued LLVM Phabricator instance.

[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

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
4927–4928

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

4930

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
1419–1422

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
8471–8477

Can you also check the end of the section here?

lib/Sema/SemaExpr.cpp
511–520

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

3935–3940

What should happen here:

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

? Or here:

int x[5][5];
(+x[2:])[2]
4075–4076

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.

4078

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
4927–4928

Fixed

4930

Fixed

lib/Sema/SemaChecking.cpp
8471–8477

Ok, added

lib/Sema/SemaExpr.cpp
3935–3940

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

4078

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
7682

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
1050–1051

Condition this behind if (LangOpts.OpenMP).

lib/AST/Stmt.cpp
18

This appears to be unused?

lib/Parse/ParseExpr.cpp
1400

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

1419

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
3989–3992

Should you IgnoreParens() here?

3994

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.

4005

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

4010–4014

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

4015

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

4060

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

4098

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.

4100

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

4134–4142

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

4153–4166

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
7682

Done.

lib/AST/ASTContext.cpp
1050–1051

Done, thanks

lib/AST/Stmt.cpp
18

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

lib/Sema/SemaExpr.cpp
3989–3992

Yes, added.

3994

Fixed, thanks.

4005

Removed

4060

Done.

4098

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.

4100

We are permitted to reject it.

4134–4142

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

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

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.