Page MenuHomePhabricator

[OPENMP50]Add basic support for array-shaping operation.
ClosedPublic

Authored by ABataev on Feb 6 2020, 10:36 AM.

Details

Summary

Added basic representation and parsing/sema handling of array-shaping
operations. Array shaping expression is an expression of form ([s0]..[sn])base,
where s0, ..., sn must be a positive integer, base - a pointer. This
expression is a kind of cast operation that converts pointer expression
into an array-like kind of expression.

Diff Detail

Event Timeline

ABataev created this revision.Feb 6 2020, 10:36 AM
Herald added a project: Restricted Project. · View Herald Transcript
martong removed a subscriber: martong.Feb 19 2020, 7:55 AM
jdoerfert added inline comments.Fri, Mar 6, 9:12 AM
clang/lib/Parse/ParseExpr.cpp
2979–3011

Out of curiosity, why do we prevent Objective C here? I doubt anyone runs it in OpenMP mode but it would be kinda cool.

clang/lib/Sema/SemaExpr.cpp
4813

Typo above.

4819

Does this trigger as well if BaseTy is a pointer type but dependent? I mean, why do we need the first check (!BaseTy->isAnyPointerType() &&)?

clang/test/OpenMP/parallel_reduction_messages.c
20

Why is there a 50 error here?

clang/test/OpenMP/task_ast_print.cpp
167

We do not complain that argc is non-constant. Is that on purpose?

ABataev marked 6 inline comments as done.Fri, Mar 6, 9:44 AM
ABataev added inline comments.
clang/lib/Parse/ParseExpr.cpp
2979–3011

Just used this to prevent the error messages from Objective C tests when enabled OpenMP by default to test that parsing work correctl in the corner cases. Will remove this check.

clang/lib/Sema/SemaExpr.cpp
4819

Not sure this is a real issue. If the type is not a pointer type, but the dependent, just skip all checks. All the required checks will be performed upon the instantiation.

clang/test/OpenMP/parallel_reduction_messages.c
20

Array shaping is not supported for the reductions

clang/test/OpenMP/task_ast_print.cpp
167

Yes, the standard allows the use of the non-constant values as the dimensions.

ABataev updated this revision to Diff 248789.Fri, Mar 6, 10:25 AM
ABataev marked an inline comment as done.

Rebase + fixes

My questions are answered, as you have the clang people in the reviewer list and I was only added by my OpenMP hook, I would them to OK this. Thanks!

Okay, so these array-shaping expressions are only allowed as operands to specific OpenMP directives? That's a plausible interpretation of "The shape-operator can appear only in clauses where it is explicitly allowed" from the spec. If it were a more general l-value expression, we could handle that just fine by building the type using ConstantArrayType/VariableArrayType as appropriate; but if the language intentionally restricts it, the placeholder approach seems fine.

clang/lib/Sema/SemaExpr.cpp
4817

Just check isTypeDependent(), none of these other conditions should interfere with type-checking.

4820

I think you should perform DefaultFunctionArrayLvalueConversion here so that e.g. arrays will decay to pointers, you load pointers from l-values, and so on. If you do so, it'll handle placeholders for you.

Do you really want to allow this to operate on non-C pointer types?

4827

I think overload placeholders need to be resolved here, too. You may have copied this code from some different place that has the ability to resolve overloads later, but in this case that's not true.

4840

You don't really care about value-dependence here, just type-dependence. You can check value-dependence before doing the constant-evaluation check below.

ABataev marked 4 inline comments as done.Wed, Mar 25, 1:20 PM

Okay, so these array-shaping expressions are only allowed as operands to specific OpenMP directives? That's a plausible interpretation of "The shape-operator can appear only in clauses where it is explicitly allowed" from the spec. If it were a more general l-value expression, we could handle that just fine by building the type using ConstantArrayType/VariableArrayType as appropriate; but if the language intentionally restricts it, the placeholder approach seems fine.

Hi John, thanks for the review!
Yes, this is not a general form of the expression, it is allowed only in a couple of clauses.
We cannot build an array type since this expression does not represent a supported array type. Sizes may be non-constant anywhere in the operation, variable array type does not support it. It requires extending of the variable array types but actually it does not worth a try, since this type is not needed at all.

clang/lib/Sema/SemaExpr.cpp
4817

Ok, will do.

4820
  1. Standard clearly states that the type of the base expression must be a pointer. I don't think that we should perform implicit type casting here, like decay to pointers, etc.
  2. It is just a simple form of checking that this is a pointer type. Since this expression is not allowed in other languages (and I filter it out at the parsing stage), I think it is ok to use the generic form of type checking.
4827

No, YouCompleteMe suggested the wrong function and I just missed it. Will fix it, thanks!

4840

Yes, just a double check to be realy-really sure :)

ABataev marked an inline comment as done.Wed, Mar 25, 1:24 PM
ABataev added inline comments.
clang/lib/Sema/SemaExpr.cpp
4820

Forgot to mention, that I thought about possible conversion here. But it may lead to some unpredictable results, like:

int a[3];
...([3][4][n])a...

Better to do not allow this kind of operation, I think.

We cannot build an array type since this expression does not represent a supported array type. Sizes may be non-constant anywhere in the operation, variable array type does not support it.

I don't think that's true; the element type of a C99 VAT can still be another VAT. The only restriction is that we don't allow a CAT of VATs — we require the outer array to still be represented as a VAT despite have a constant immediate bound — but that's just a representational restriction and doesn't restrict what we can express. But it doesn't matter because, as you say, we can and should use a placeholder here in order to enforce the use-restriction.

clang/lib/Sema/SemaExpr.cpp
4820

Standard clearly states that the type of the base expression must be a pointer. I don't think that we should perform implicit type casting here, like decay to pointers, etc.

The wording of the C/C++ standard is that expressions of array type decay except in certain syntactic positions. Such expressions then *do* have pointer type for this purpose. If precedent is to forbid array references, so be it, but that's not consistent with the normal language behavior.

It is just a simple form of checking that this is a pointer type. Since this expression is not allowed in other languages (and I filter it out at the parsing stage), I think it is ok to use the generic form of type checking.

If your intent is that this only applies only to C pointers, you should just check isPointerType() instead of isAnyPointerType(), which exists for the sole purpose of also including Objective-C pointers.

4820

You can still have pointers to arrays without suppressing array decay.

4840

Type-dependence implies value-dependence anyway.

ABataev updated this revision to Diff 252700.Wed, Mar 25, 4:01 PM

Rebase + fixes.

Okay, if you think that suppress array decay is the right behavior here, that's fine with me. Please check the behavior of other OpenMP implementations, though.

clang/lib/Sema/SemaExpr.cpp
4848

You do still need an isValueDependent() check here, and you should add a test that exercises it. (A sizeof(T) argument is probably good enough.)

ABataev updated this revision to Diff 252859.Thu, Mar 26, 8:34 AM

Rebase + fixes.

rjmccall accepted this revision.Thu, Mar 26, 8:43 AM

Thanks, LGTM.

This revision is now accepted and ready to land.Thu, Mar 26, 8:43 AM
This revision was automatically updated to reflect the committed changes.