Semantic checks for Select-rank construct(#R1148)

[Sameeranjoshi]

Semantic checks for R1148

[klausler]

A good start.

[Sameeranjoshi]

A good start.

Done

[Sameeranjoshi]

The specification doesn't speak on what happens when selector is an expression.
For example SELECT RANK(a+b) or SELECT RANK(0+1) ....
Is if (std::holds_alternative<parser::Expr>(selectRankStmtSel.u)) always expected to throw a semantic error?

[klausler]

The specification doesn't speak on what happens when selector is an expression.
For example SELECT RANK(a+b) or SELECT RANK(0+1) ....
Is if (std::holds_alternative<parser::Expr>(selectRankStmtSel.u)) always expected to throw a semantic error?

R1150 requires that the expression be a scalar integer constant (which will be packaged as an Expr<SomeType>).

[Sameeranjoshi]

The specification doesn't speak on what happens when selector is an expression.
For example SELECT RANK(a+b) or SELECT RANK(0+1) ....
Is if (std::holds_alternative<parser::Expr>(selectRankStmtSel.u)) always expected to throw a semantic error?

R1150 requires that the expression be a scalar integer constant (which will be packaged as an Expr<SomeType>).

I am unable to figure out how to handle to TODO case.
If selector is an expression
Can you please point how to move ahead?
Thanks

[klausler]

It would be a better test here if y were to be used in some context that required it to be a scalar variable. Similarly, in the other RANK cases, use the associate-name in some context that will elicit an error if the required rank were incorrect.

[Sameeranjoshi]

I have added some tests are they as expected ones ?

[klausler]

No. Instead of just having a PRINT statement, try using the SELECT RANK construct entity in the test in some way that would work only when the construct entity has the expected rank, using something with a compile-time test of the rank.

SELECT RANK(x)
RANK(0)
  j = INT(0, KIND=MERGE(KIND(0), -1, RANK(x) == 0)) ! will fail when RANK(x) is not zero here
END SELECT

[Sameeranjoshi]

For the patch submitted(currently passed a non zero value to pass the drone.io build) , test Fails with below error, when the rank is 0, which is supposed to fail instead when rank is not zero.
_../f18/test/semantics/select-rank.f90:39:11: error: 'kind=' argument must be a constant scalar integer whose value is a supported kind for the intrinsic result type

j = INT(0, KIND=MERGE(KIND(0), -1, RANK(x) == 0)) ! will fail when RANK(x) is not zero ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ _
Is this an issue with the RANK construct or select rank ?

[klausler]

That seems to indicate a problem with the semantics or name resolution of xin theRANK. If the rank of xin that context were zero, as it should be, theKIND=argument would have had a valid value. You can test that hypothesis by changing==to/=` in that expression and seeing whether it (incorrectly) does not emit an error.

[Sameeranjoshi]

Conformed for above discussed test with suggested changes,
j = INT(0, KIND=MERGE(KIND(0), -1, RANK(x) /= 0)) !Is evaluating to true, which means RANK is not resolving properly the value of x.

[klausler]

Is this new field relevant to the shape analysis code in lib/Evaluate/shape.cpp? I think that it must be, so that code should be extended to use the specific case ranks so that operations that depend on the shape of an array will be compiled correctly.

[Sameeranjoshi]

Can you please explain more on what do you mean by

new field relevant to the shape analysis code

The tests other than select rank which depend on shape of array seem to be successful for ninja test check-all

What I can understand from comments is that,
MaybeExpr expr_ private member function in class AssocEntityDetails is being used in lib/Evaluate/shape.cpp inside function GetShape()
and my patch doesn't seem to handle the expr_ inside SetRankFromParserNode but is handled in SetTypeFromAssociation

[klausler]

Make sure that your tests for SELECT RANK include operations in the rank cases that will force the compiler to analyze the shape of the array at the specific rank. Use FORALL statements, the SHAPE intrinsic, array assignments, &c. There is probably code in the shape analysis framework that will need to be updated to handle this new representation of construct entities correctly, using the rank of the construct entity and the descriptor of the assumed-rank dummy argument for inquiries.

[Sameeranjoshi]

@klausler I have tried to formulate some tests from my understanding of Fortran language.
Pasting the godbolt.org links for keeping the comments clean.
Can you please suggest if they need more changes?
https://godbolt.org/z/UiRcRg = SHAPE intrinsics
https://godbolt.org/z/oW2Zyl = SHAPE intrinsics
https://godbolt.org/z/N86jMD = using FORALL statement
https://godbolt.org/z/YKWY9A = using array assignment statement
https://godbolt.org/z/raN-5f = using LOC for address of array

[Sameeranjoshi]

@klausler I have tried to formulate some tests from my understanding of Fortran language.
Pasting the godbolt.org links for keeping the comments clean.
Can you please suggest if they need more changes?
https://godbolt.org/z/UiRcRg = SHAPE intrinsics
https://godbolt.org/z/oW2Zyl = SHAPE intrinsics
https://godbolt.org/z/N86jMD = using FORALL statement
https://godbolt.org/z/YKWY9A = using array assignment statement
https://godbolt.org/z/raN-5f = using LOC for address of array

If the tests seem to be written as expected, I see all passing except the following condition.

 select rank (arr) 
            rank(2)
                j = INT(0, KIND=MERGE(KIND(0), -1, RANK(arr) == SIZE(SHAPE(arr)))) !here aarr is dummy 
        end select

Checking
lib/Evaluate/shape.cpp at this function
Seems like assoc.expr() might not have rank information.Is that the issue you see?
@klausler can you please point what do you expect this to return ? as we don't have shape_ in AssocEntityDetails. to fill in the shape vector to return.

[klausler]

Seems like assoc.expr() might not have rank information.Is that the issue you see?
@klausler can you please point what do you expect this to return ? as we don't have shape_ in AssocEntityDetails. to fill in the shape vector to return.

assoc.expr() is the assumed-rank dummy argument itself.

Within a RANK case, shape analysis should return a shape vector of the appropriate rank populated with descriptor inquiries into the extents of the leading dimensions of the assumed-rank dummy argument.

[Sameeranjoshi]

@klausler thanks for explaining, but there seems to be regression for other tests and the select-rank cases, not sure why are they regressing(They tend to Abort the compiler).
Did I create a wrong patch?

[klausler]

What about association entities that are not RANK cases, like ASSOCIATE(x=>array)? This code used to work for them, and now it won't.

[Sameeranjoshi]

Thanks, I have added a guard using if (!assoc.associationRank()), the regression pass now.
Is there any related function you know apart from above usage, or suggest some other design pattern you think, like adding something in class AssocEntityDetails like IsAssumedRank() function similar to ObjectEentityDetails? I think the only way we can identify select rank is the -fdebug-dump-symbols will have a new rank: field.

[klausler]

This is not a good name for this file. Fortran has three different SELECT statements and this file only checks one of the three.

[Sameeranjoshi]

How about check-select-rank.[cpp|h]?

[klausler]

Don't test RANK(arr) and SIZE(SHAPE(arr)) against each other when you know what their values should be (2). They could both be wrong.

[klausler]

I don't understand what's being tested here.

[Sameeranjoshi]

Ok, I was trying to test with a local copy and the dummy arguments.
Remove this test?

[klausler]

This test is just working with a local array and doesn't depend on the effective shape of the assumed-rank array at all.

[Sameeranjoshi]

Ok, changing it to arr.

[Sameeranjoshi]

@klausler I have incorporated most of the changes.
Some queries I have asked in for the review comments.
Thank you.

[Sameeranjoshi]

Ping..

[Sameeranjoshi]

Merged in llvm-project/master.