This is an archive of the discontinued LLVM Phabricator instance.

[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.Mar 6 2020, 9:12 AM
clang/lib/Parse/ParseExpr.cpp
2891–2923

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
4744

Typo above.

4750

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
8

Why is there a 50 error here?

clang/test/OpenMP/task_ast_print.cpp
160

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

ABataev marked 6 inline comments as done.Mar 6 2020, 9:44 AM
ABataev added inline comments.
clang/lib/Parse/ParseExpr.cpp
2891–2923

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
4750

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
8

Array shaping is not supported for the reductions

clang/test/OpenMP/task_ast_print.cpp
160

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

ABataev updated this revision to Diff 248789.Mar 6 2020, 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
4748

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

4751

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?

4758

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.

4771

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.Mar 25 2020, 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
4748

Ok, will do.

4751
  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.
4758

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

4771

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

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

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
4751

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.

4751

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

4771

Type-dependence implies value-dependence anyway.

ABataev updated this revision to Diff 252700.Mar 25 2020, 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
4779

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.Mar 26 2020, 8:34 AM

Rebase + fixes.

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

Thanks, LGTM.

This revision is now accepted and ready to land.Mar 26 2020, 8:43 AM
This revision was automatically updated to reflect the committed changes.
cchen added a subscriber: cchen.Oct 1 2020, 3:50 PM

@ABataev, the below test is extracted from Sollve test suite and Clang now emit:

test.c:17:35: error: subscripted value is not an array or pointer
    #pragma omp target update to( (([N][N])foo)[1:M] )
                                  ^~~~~~~~~~~~~
test.c:17:5: error: expected at least one 'to' clause or 'from' clause specified to '#pragma omp target update'
    #pragma omp target update to( (([N][N])foo)[1:M] )

This error message came from the ActOnOMPArraySectionExpr which is called inside ParsePostfixExpressionSuffix. The issue is that the base expression in ActOnOMPArraySectionExpr looks like:

ParenExpr 0x122859be0 '<OpenMP array shaping type>' lvalue
`-OMPArrayShapingExpr 0x122859b98 '<OpenMP array shaping type>' lvalue
  |-IntegerLiteral 0x122859b38 'int' 5
  |-IntegerLiteral 0x122859b58 'int' 5
  `-DeclRefExpr 0x122859b78 'int *' lvalue Var 0x1228599d0 'foo' 'int *'

which is not a base that we would expect in an array section expr. I've tried relaxing the base type check in ActOnOMPArraySectionExpr but not sure it's the way to go. (or should I just extract the DeclReExpr from ArrayShapingExpr before calling ActOnOMPArraySectionExpr?)

#define N 5
#define M 3

int main(void) {
    int tmp[N][N];
    for(int i=0; i<N; i++)
        for(int j=0; j<N; j++)
            tmp[i][j] = N*i + j;

    int *foo = &tmp[0][0];

    // This compiles just fine
    //#pragma omp target update to( ([N][N])foo )

    // This is rejected by the compiler
    #pragma omp target update to( (([N][N])foo)[1:M] )
}

@ABataev, the below test is extracted from Sollve test suite and Clang now emit:

test.c:17:35: error: subscripted value is not an array or pointer
    #pragma omp target update to( (([N][N])foo)[1:M] )
                                  ^~~~~~~~~~~~~
test.c:17:5: error: expected at least one 'to' clause or 'from' clause specified to '#pragma omp target update'
    #pragma omp target update to( (([N][N])foo)[1:M] )

This error message came from the ActOnOMPArraySectionExpr which is called inside ParsePostfixExpressionSuffix. The issue is that the base expression in ActOnOMPArraySectionExpr looks like:

ParenExpr 0x122859be0 '<OpenMP array shaping type>' lvalue
`-OMPArrayShapingExpr 0x122859b98 '<OpenMP array shaping type>' lvalue
  |-IntegerLiteral 0x122859b38 'int' 5
  |-IntegerLiteral 0x122859b58 'int' 5
  `-DeclRefExpr 0x122859b78 'int *' lvalue Var 0x1228599d0 'foo' 'int *'

which is not a base that we would expect in an array section expr. I've tried relaxing the base type check in ActOnOMPArraySectionExpr but not sure it's the way to go. (or should I just extract the DeclReExpr from ArrayShapingExpr before calling ActOnOMPArraySectionExpr?)

#define N 5
#define M 3

int main(void) {
    int tmp[N][N];
    for(int i=0; i<N; i++)
        for(int j=0; j<N; j++)
            tmp[i][j] = N*i + j;

    int *foo = &tmp[0][0];

    // This compiles just fine
    //#pragma omp target update to( ([N][N])foo )

    // This is rejected by the compiler
    #pragma omp target update to( (([N][N])foo)[1:M] )
}

I don't think it is allowed by the standard.

According to the standard, The shape-operator can appear only in clauses where it is explicitly allowed.
In this case, array shaping is used as a base expression of array section (or subscript) expression, which does not meet the standard. Tje array sjaping operation is not used in clause, instead it is used as a base subexpression of another expression.

cchen added a comment.Oct 2 2020, 9:07 AM

@ABataev, the below test is extracted from Sollve test suite and Clang now emit:

test.c:17:35: error: subscripted value is not an array or pointer
    #pragma omp target update to( (([N][N])foo)[1:M] )
                                  ^~~~~~~~~~~~~
test.c:17:5: error: expected at least one 'to' clause or 'from' clause specified to '#pragma omp target update'
    #pragma omp target update to( (([N][N])foo)[1:M] )

This error message came from the ActOnOMPArraySectionExpr which is called inside ParsePostfixExpressionSuffix. The issue is that the base expression in ActOnOMPArraySectionExpr looks like:

ParenExpr 0x122859be0 '<OpenMP array shaping type>' lvalue
`-OMPArrayShapingExpr 0x122859b98 '<OpenMP array shaping type>' lvalue
  |-IntegerLiteral 0x122859b38 'int' 5
  |-IntegerLiteral 0x122859b58 'int' 5
  `-DeclRefExpr 0x122859b78 'int *' lvalue Var 0x1228599d0 'foo' 'int *'

which is not a base that we would expect in an array section expr. I've tried relaxing the base type check in ActOnOMPArraySectionExpr but not sure it's the way to go. (or should I just extract the DeclReExpr from ArrayShapingExpr before calling ActOnOMPArraySectionExpr?)

#define N 5
#define M 3

int main(void) {
    int tmp[N][N];
    for(int i=0; i<N; i++)
        for(int j=0; j<N; j++)
            tmp[i][j] = N*i + j;

    int *foo = &tmp[0][0];

    // This compiles just fine
    //#pragma omp target update to( ([N][N])foo )

    // This is rejected by the compiler
    #pragma omp target update to( (([N][N])foo)[1:M] )
}

I don't think it is allowed by the standard.

According to the standard, The shape-operator can appear only in clauses where it is explicitly allowed.
In this case, array shaping is used as a base expression of array section (or subscript) expression, which does not meet the standard. Tje array sjaping operation is not used in clause, instead it is used as a base subexpression of another expression.

In OpenMP 5.0 [2.12.6, target update construct, Restrictions, C/C++, p.1] The list items that appear in the to or from clauses may use shape-operators.
Also, in the array shaping section in https://github.com/OpenMP/Examples, the example is also illustrated with the same usage:

...
S-17 // update boundary points (two columns of 2D array) on the host
S-18 // pointer is shaped to 2D array using the shape-operator
S-19 #pragma omp target update from( (([nx][ny+2])a)[0:nx][1], (([nx][ny+2])a)[0:nx][ny] )
...

@ABataev, the below test is extracted from Sollve test suite and Clang now emit:

test.c:17:35: error: subscripted value is not an array or pointer
    #pragma omp target update to( (([N][N])foo)[1:M] )
                                  ^~~~~~~~~~~~~
test.c:17:5: error: expected at least one 'to' clause or 'from' clause specified to '#pragma omp target update'
    #pragma omp target update to( (([N][N])foo)[1:M] )

This error message came from the ActOnOMPArraySectionExpr which is called inside ParsePostfixExpressionSuffix. The issue is that the base expression in ActOnOMPArraySectionExpr looks like:

ParenExpr 0x122859be0 '<OpenMP array shaping type>' lvalue
`-OMPArrayShapingExpr 0x122859b98 '<OpenMP array shaping type>' lvalue
  |-IntegerLiteral 0x122859b38 'int' 5
  |-IntegerLiteral 0x122859b58 'int' 5
  `-DeclRefExpr 0x122859b78 'int *' lvalue Var 0x1228599d0 'foo' 'int *'

which is not a base that we would expect in an array section expr. I've tried relaxing the base type check in ActOnOMPArraySectionExpr but not sure it's the way to go. (or should I just extract the DeclReExpr from ArrayShapingExpr before calling ActOnOMPArraySectionExpr?)

#define N 5
#define M 3

int main(void) {
    int tmp[N][N];
    for(int i=0; i<N; i++)
        for(int j=0; j<N; j++)
            tmp[i][j] = N*i + j;

    int *foo = &tmp[0][0];

    // This compiles just fine
    //#pragma omp target update to( ([N][N])foo )

    // This is rejected by the compiler
    #pragma omp target update to( (([N][N])foo)[1:M] )
}

I don't think it is allowed by the standard.

According to the standard, The shape-operator can appear only in clauses where it is explicitly allowed.
In this case, array shaping is used as a base expression of array section (or subscript) expression, which does not meet the standard. Tje array sjaping operation is not used in clause, instead it is used as a base subexpression of another expression.

In OpenMP 5.0 [2.12.6, target update construct, Restrictions, C/C++, p.1] The list items that appear in the to or from clauses may use shape-operators.
Also, in the array shaping section in https://github.com/OpenMP/Examples, the example is also illustrated with the same usage:

...
S-17 // update boundary points (two columns of 2D array) on the host
S-18 // pointer is shaped to 2D array using the shape-operator
S-19 #pragma omp target update from( (([nx][ny+2])a)[0:nx][1], (([nx][ny+2])a)[0:nx][ny] )
...

Then just need to fix it, if examples document has this example.

@ABataev, the below test is extracted from Sollve test suite and Clang now emit:

test.c:17:35: error: subscripted value is not an array or pointer
    #pragma omp target update to( (([N][N])foo)[1:M] )
                                  ^~~~~~~~~~~~~
test.c:17:5: error: expected at least one 'to' clause or 'from' clause specified to '#pragma omp target update'
    #pragma omp target update to( (([N][N])foo)[1:M] )

This error message came from the ActOnOMPArraySectionExpr which is called inside ParsePostfixExpressionSuffix. The issue is that the base expression in ActOnOMPArraySectionExpr looks like:

ParenExpr 0x122859be0 '<OpenMP array shaping type>' lvalue
`-OMPArrayShapingExpr 0x122859b98 '<OpenMP array shaping type>' lvalue
  |-IntegerLiteral 0x122859b38 'int' 5
  |-IntegerLiteral 0x122859b58 'int' 5
  `-DeclRefExpr 0x122859b78 'int *' lvalue Var 0x1228599d0 'foo' 'int *'

which is not a base that we would expect in an array section expr. I've tried relaxing the base type check in ActOnOMPArraySectionExpr but not sure it's the way to go. (or should I just extract the DeclReExpr from ArrayShapingExpr before calling ActOnOMPArraySectionExpr?)

#define N 5
#define M 3

int main(void) {
    int tmp[N][N];
    for(int i=0; i<N; i++)
        for(int j=0; j<N; j++)
            tmp[i][j] = N*i + j;

    int *foo = &tmp[0][0];

    // This compiles just fine
    //#pragma omp target update to( ([N][N])foo )

    // This is rejected by the compiler
    #pragma omp target update to( (([N][N])foo)[1:M] )
}

I don't think it is allowed by the standard.

According to the standard, The shape-operator can appear only in clauses where it is explicitly allowed.
In this case, array shaping is used as a base expression of array section (or subscript) expression, which does not meet the standard. Tje array sjaping operation is not used in clause, instead it is used as a base subexpression of another expression.

In OpenMP 5.0 [2.12.6, target update construct, Restrictions, C/C++, p.1] The list items that appear in the to or from clauses may use shape-operators.
Also, in the array shaping section in https://github.com/OpenMP/Examples, the example is also illustrated with the same usage:

...
S-17 // update boundary points (two columns of 2D array) on the host
S-18 // pointer is shaped to 2D array using the shape-operator
S-19 #pragma omp target update from( (([nx][ny+2])a)[0:nx][1], (([nx][ny+2])a)[0:nx][ny] )
...

Then just need to fix it, if examples document has this example.

Was this ever followed up on and fixed?

Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: steakhal. · View Herald Transcript