[flang] Add a semantics test for co_reduce

[rouson]

Summary

This patch checks for valid and invalid forms of calls to the collective subroutine co_reduce.

[rouson]

1. Add co_reduce test to expectedly failed list.
2. Replaced non-ASCII characters.
3. Correct expected error message.
4. Update run directive to use test_errors.py.

[rouson]

@kiranchandramohan with your approval and that of @ktras, I just pushed differential D113076. Could you review and approve this one along with differential D113077, D113083, and D113084? Also, could you point me to a policy on what approvals are needed in order to push commits to main?

[ekieri]

In D113086#3139018, @rouson wrote:

@kiranchandramohan with your approval and that of @ktras, I just pushed differential D113076. Could you review and approve this one along with differential D113077, D113083, and D113084? Also, could you point me to a policy on what approvals are needed in order to push commits to main?

Hi, and thanks for working on this! Please see https://llvm.org/docs/DeveloperPolicy.html and https://llvm.org/docs/CodeReview.html. In summary, you may push when at least one reviewer has approved the patch, and all comments (by anyone) have been addressed. It can also be nice to let at least 24h pass between the first post of the patch and the push, to give people across all time zones a chance to comment, especially if the patch may be controversial in some way.

[kiranchandramohan]

As @ekieri says you can submit with approval from another member of the community who works in this area.
If you have questions, the expert in this area (Parsing, Semantics, Runtime) is @klausler. You may add him for review.

Regarding the tests, I would like to understand the scope. I see a lot of restrictions for the arguments like the following. To exhaustively test, we will have to give A as polymorphic and coindexed, the OPERATION with more than two arguments, disallowed kind of results and arguments etc.
co_reduce(A, OPERATION [,RESULT_IMAGE, STAT, ERRMSG)
i. A should not be polymorphic, shall not be coindexed
ii. OPERATION shall be pure with two arguments, result and each argument shall be a scalar, nonallocatable, nonpointer, nonpolymorphic data object, with the same type and parameters as A. Similarity of properties (Volatile, Target, Value) for both arguments. The OPERATION shall be associative.

If this covers only a subset of the situations please mention that in the comment. We can build up the tests over time.

[clementval]

flang/test/Semantics/collectives05.f90

5

What is the evaluation stage?

[ekieri]

It would be nice with a comment citing where in the standard the interface of this intrinsic is specified.

Are you (or do you know of someone who is) planning to write the actual semantics checks as well in the near future? I am a little concerned that otherwise the developer who does that one day will be unaware of these tests, and implements additional tests of their own, leaving your tests with XFAIL.

Also, your test contains trailing whitespace. Is this intentional? The coding standards [1] discourages that, and while they primarily concern the c++ code, the argument provided is no less valid for test cases in fortran. On the other hand, @klausler made the (also valid) argument in D113077 that variations in style improves test coverage of the parser. I am fine with you doing it either way, but encourage you to make a conscious decision about it.

[1] https://llvm.org/docs/CodingStandards.html#whitespace

[ekieri]

flang/test/Semantics/collectives05.f90

42

Please start error messages with a captial letter, as per [1]. (I suppose that overrides [2], which requires the opposite.) Same things on line 45.

[1] https://flang.llvm.org/docs/C++style.html#error-messages
[2] https://llvm.org/docs/CodingStandards.html#error-and-warning-messages

[ktras]

In D113086#3145204, @ekieri wrote:

It would be nice with a comment citing where in the standard the interface of this intrinsic is specified.

Are you (or do you know of someone who is) planning to write the actual semantics checks as well in the near future? I am a little concerned that otherwise the developer who does that one day will be unaware of these tests, and implements additional tests of their own, leaving your tests with XFAIL.

Also, your test contains trailing whitespace. Is this intentional? The coding standards [1] discourages that, and while they primarily concern the c++ code, the argument provided is no less valid for test cases in fortran. On the other hand, @klausler made the (also valid) argument in D113077 that variations in style improves test coverage of the parser. I am fine with you doing it either way, but encourage you to make a conscious decision about it.

[1] https://llvm.org/docs/CodingStandards.html#whitespace

@ekieri, in response to one of your questions, I am currently working on adding the collectives to flang/lib/Evaluate/intrinsics.cpp. When I submit patches, they will include changes to the corresponding collective test to remove the XFAIL directive. However, regardless of who works on the collectives, when a test with an XFAIL directive does not fail, the test will be listed as Unexpectedly Passed when running the test suite. This should bring any developer's attention to these tests that @rouson is submitting.

[ktras]

flang/test/Semantics/collectives05.f90

42

@rouson is not creating error messages here, he is matching the error message that will be produced by the compiler once collectives have been added to the list of intrinsics. If the error message not beginning with a capital letter is an issue, the source code that currently procedures this message would need to be edited. I personally would think that that might be outside the scope of this patch, as it is just adding a test, not editing source code.

[ekieri]

In D113086#3162638, @ktras wrote:

In D113086#3145204, @ekieri wrote:

It would be nice with a comment citing where in the standard the interface of this intrinsic is specified.

Are you (or do you know of someone who is) planning to write the actual semantics checks as well in the near future? I am a little concerned that otherwise the developer who does that one day will be unaware of these tests, and implements additional tests of their own, leaving your tests with XFAIL.

Also, your test contains trailing whitespace. Is this intentional? The coding standards [1] discourages that, and while they primarily concern the c++ code, the argument provided is no less valid for test cases in fortran. On the other hand, @klausler made the (also valid) argument in D113077 that variations in style improves test coverage of the parser. I am fine with you doing it either way, but encourage you to make a conscious decision about it.

[1] https://llvm.org/docs/CodingStandards.html#whitespace

@ekieri, in response to one of your questions, I am currently working on adding the collectives to flang/lib/Evaluate/intrinsics.cpp. When I submit patches, they will include changes to the corresponding collective test to remove the XFAIL directive. However, regardless of who works on the collectives, when a test with an XFAIL directive does not fail, the test will be listed as Unexpectedly Passed when running the test suite. This should bring any developer's attention to these tests that @rouson is submitting.

Great, thanks, as you are working on that I see no issue. I also thought that 'Unexpectedly Passed' was unlikely to trigger due to differences in the error messages, but as all messages are pre-existing (as you kindly pointed out) I admit this risk is small. But still, good that you are working on the checks!

flang/test/Semantics/collectives05.f90

42

Indeed, sorry, and thanks for pointing that out. I agree completely, fixing that kind of formatting is another patch.

[ktras]

Great, thanks, as you are working on that I see no issue. I also thought that 'Unexpectedly Passed' was unlikely to trigger due to differences in the error messages, but as all messages are pre-existing (as you kindly pointed out) I admit this risk is small. But still, good that you are working on the checks!

Since I have a little experience looking into the flang source code, as @rouson was writing his tests, I made some changes locally so that I could provide him with the error messages that would occur once the intrinsic he was working on was added to flang. However, @ekieri, you make a great point that I hadn't thought through that, if the error messages the test expects were wrong, then once a developer adds that intrinsic, the test would continue to expectedly fail, rather than unexpectedly pass. Since I am planning on working on these, this doesn't seem to be a concern, but it is a possibility that is good to be aware of, thanks for the feedback!

[rouson]

All comments have been addressed:

1. Trailing whitespace has been removed.
2. The section of the standard that defines the co_reduce interface is cited in a comment.
3. All statistically checkable restrictions are now checked.

[ekieri]

Thanks for addressing my comments! I noted an oversight inline, and wonder about the intended test coverage, otherwise this looks good I think.

In your update note, you say that all statically checkable restrictions are covered. However, a few are still missing if I'm not mistaken. For example, I do not see any checks for polymorphic 'A', or type parameters for the arguments and result of 'operation'. Also, the semantics of 'result_image', 'stat', and 'errmsg' appear untested. Not a blocker for me. As @kiranchandramohan said, we can build up the tests over time. But if you commit it incomplete, please include a comment describing its coverage.

'stat' and 'errmsg' appear as arguments in several intrinsics with the same constraints. Could it be preferable to check them, for all intrinsics where they appear, in a separate test? If so, it would be good to reference that test from here with a comment (todo-note for now, I presume).

flang/test/Semantics/collectives05.f90

95

This function is not defined.

[rouson]

updated patch based on comments from @ekieri

[rouson]

@ekieri Thanks for the helpful suggestions. The latest update to the patch includes a check for polymorphic 'A' and a check for matching type parameters for the arguments and result of 'operation'. The latter test has both arguments and the result all not matching a kind-type parameter of 'A'. I will upload one more patch soon with checks for the semantics of 'result_image', 'stat', and 'errmsg'.

[rouson]

The latest version of the patch addresses all reviewer comments.

[ekieri]

I'll try to find time for another look within the next few days. Please don't forget to answer @clementval 's inline comment, and mark mine as done if you fixed it. And I would prefer if you add a TODO comment in the file header about tests for 'result_image', 'stat', and 'errmsg'.

[ekieri]

In D113086#3183917, @ekieri wrote:

I'll try to find time for another look within the next few days. Please don't forget to answer @clementval 's inline comment, and mark mine as done if you fixed it. And I would prefer if you add a TODO comment in the file header about tests for 'result_image', 'stat', and 'errmsg'.

Now I've read it properly, and see I misunderstood you. The tests for result_image, stat, errmsg are there, and look good. I interpreted you as if you were to put them in a different revision (probably because I would have done it that way), but they're here, they're fine, and I think it works just as good to do it this way. Sorry about that.

I added two tiny nits inline (typos in comments). LGTM, as long as @clementval's question is answered. But please give it a day or so in case Valentin or @kiranchandramohan, or somebody else, has something to add. Thanks for working on this!

flang/test/Semantics/collectives05.f90

96

120

[rouson]

flang/test/Semantics/collectives05.f90

95

@ekieri I deleted the line of code that contained the undefined function.

[ktras]

flang/test/Semantics/collectives05.f90

5

@rouson added this comment because I plan to add the collective subroutines to the list of intrinsic functions in flang/lib/Evaluate/intrinsics.cpp. If the term evaluation stage does not have significant meaning, I will suggest to Damian to change the to-do comment.

[rouson]

Handled all review comments:

1. Fixed two grammatical errors in comments.
2. Clarified the To Do comment.

flang/test/Semantics/collectives05.f90

+! RUN: %python %S/test_errors.py %s %flang_fc1
+! XFAIL: *
+! This test checks for semantic errors in co_reduce subroutine calls based on
+! the co_reduce interface defined in section 16.9.49 of the Fortran 2018 standard.
+! To Do: add co_reduce to the list of intrinsics
+
+module foo_m
+  implicit none
+
+  type foo_t
+    integer :: n=0
+  contains
+    procedure :: derived_type_op
+    generic :: operator(+) => derived_type_op
+  end type
+
+contains
+
+  pure function derived_type_op(lhs, rhs) result(lhs_op_rhs)
+    class(foo_t), intent(in) :: lhs, rhs
+    type(foo_t) lhs_op_rhs
+    lhs_op_rhs%n = lhs%n + rhs%n
+  end function
+
+end module foo_m
+
+program main
+  use foo_m, only : foo_t
+  implicit none
+
+  type(foo_t) foo
+  class(foo_t), allocatable :: polymorphic
+  integer i, status, integer_array(1)
+  real x
+  real vector(1)
+  real array(1,1,1, 1,1,1, 1,1,1, 1,1,1, 1,1,1)
+  character(len=1) string, message, character_array(1)
+  integer coindexed[*]
+  logical bool
+
+  ! correct calls, should produce no errors
+  call co_reduce(i,      int_op)
+  call co_reduce(i,      int_op,                            status)
+  call co_reduce(i,      int_op,                            stat=status)
+  call co_reduce(i,      int_op,                                         errmsg=message)
+  call co_reduce(i,      int_op,                            stat=status, errmsg=message)
+  call co_reduce(i,      int_op,            result_image=1, stat=status, errmsg=message)
+  call co_reduce(i,      operation=int_op,  result_image=1, stat=status, errmsg=message)
+  call co_reduce(a=i,    operation=int_op,  result_image=1, stat=status, errmsg=message)
+  call co_reduce(array,  operation=real_op, result_image=1, stat=status, errmsg=message)
+  call co_reduce(vector, operation=real_op, result_image=1, stat=status, errmsg=message)
+  call co_reduce(string, operation=char_op, result_image=1, stat=status, errmsg=message)
+  call co_reduce(foo,    operation=left,    result_image=1, stat=status, errmsg=message)
+
+  allocate(foo_t :: polymorphic)
+
+  ! Test all statically verifiable semantic requirements on co_reduce arguments
+  ! Note: We cannot check requirements that relate to "corresponding references." 
+  ! References can correspond only if they execute on differing images.  A code that
+  ! executes in a single image might be standard-conforming even if the same code
+  ! executing in multiple images is not.
+
+  ! argument 'a' cannot be polymorphic
+  !ERROR: to be determined
+  call co_reduce(polymorphic, derived_type_op)
+
+  ! argument 'a' cannot be coindexed
+  !ERROR: (message to be determined)
+  call co_reduce(coindexed[1], int_op)
+
+  ! argument 'a' is intent(inout)
+  !ERROR: (message to be determined)
+  call co_reduce(i + 1, int_op)
+
+  ! operation must be a pure function
+  !ERROR: (message to be determined)
+  call co_reduce(i, operation=not_pure)
+
+  ! operation must have exactly two arguments
+  !ERROR: (message to be determined)
+  call co_reduce(i, too_many_args)
+
+  ! operation result must be a scalar
+  !ERROR: (message to be determined)
+  call co_reduce(i, array_result)
+
+  ! operation result must be non-allocatable
+  !ERROR: (message to be determined)
+  call co_reduce(i, allocatable_result)
+
+  ! operation result must be non-pointer
+  !ERROR: (message to be determined)
+  call co_reduce(i, pointer_result)
+
+  ! operation's arguments must be scalars
+  !ERROR: (message to be determined)
+  call co_reduce(i, array_args)
+
+  ! operation arguments must be non-allocatable
+  !ERROR: (message to be determined)
+  call co_reduce(i, allocatable_args)
+
+  ! operation arguments must be non-pointer
+  !ERROR: (message to be determined)
+  call co_reduce(i, pointer_args)
+
+  ! operation arguments must be non-polymorphic
+  !ERROR: (message to be determined)
+  call co_reduce(i, polymorphic_args)
+
+  ! operation: type of 'operation' result and arguments must match type of argument 'a'
+  !ERROR: (message to be determined)
+  call co_reduce(i, real_op)
+
+  ! operation: kind type parameter of 'operation' result and arguments must match kind type parameter of argument 'a'
+  !ERROR: (message to be determined)
+  call co_reduce(x, double_precision_op)
+
+  ! arguments must be non-optional
+  !ERROR: (message to be determined)
+  call co_reduce(i, optional_args)
+
+  ! if one argument is asynchronous, the other must be also
+  !ERROR: (message to be determined)
+  call co_reduce(i, asynchronous_mismatch)
+
+  ! if one argument is a target, the other must be also
+  !ERROR: (message to be determined)
+  call co_reduce(i, target_mismatch)
+
+  ! if one argument has the value attribute, the other must have it also
+  !ERROR: (message to be determined)
+  call co_reduce(i, value_mismatch)
+
+  ! result_image argument must be an integer scalar
+  !ERROR: to be determined
+  call co_reduce(i, int_op, result_image=integer_array)
+
+  ! result_image argument must be an integer
+  !ERROR: to be determined
+  call co_reduce(i, int_op, result_image=bool)
+
+  ! stat not allowed to be coindexed
+  !ERROR: to be determined
+  call co_reduce(i, int_op, stat=coindexed[1])
+
+  ! stat argument must be an integer scalar
+  !ERROR: to be determined
+  call co_reduce(i, int_op, result_image=1, stat=integer_array)
+
+  ! stat argument has incorrect type
+  !ERROR: Actual argument for 'stat=' has bad type 'CHARACTER(KIND=1,LEN=1_8)'
+  call co_reduce(i, int_op, result_image=1, string)
+
+  ! stat argument is intent(out)
+  !ERROR: to be determined
+  call co_reduce(i, int_op, result_image=1, stat=1+1)
+
+  ! errmsg argument must not be coindexed
+  !ERROR: to be determined
+  call co_reduce(i, int_op, result_image=1, stat=status, errmsg=conindexed_string[1])
+
+  ! errmsg argument must be a scalar
+  !ERROR: to be determined
+  call co_reduce(i, int_op, result_image=1, stat=status, errmsg=character_array)
+
+  ! errmsg argument must be a character
+  !ERROR: to be determined
+  call co_reduce(i, int_op, result_image=1, stat=status, errmsg=i)
+
+  ! errmsg argument is intent(inout)
+  !ERROR: to be determined
+  call co_reduce(i, int_op, result_image=1, stat=status, errmsg="literal constant")
+
+  ! too many arguments to the co_reduce() call
+  !ERROR: too many actual arguments for intrinsic 'co_reduce'
+  call co_reduce(i, int_op, result_image=1, stat=status, errmsg=message, 3.4)
+
+  ! non-existent keyword argument
+  !ERROR: unknown keyword argument to intrinsic 'co_reduce'
+  call co_reduce(fake=3.4)
+
+contains
+
+  pure function left(lhs, rhs) result(lhs_op_rhs)
+    type(foo_t), intent(in)  :: lhs, rhs
+    type(foo_t) :: lhs_op_rhs
+    lhs_op_rhs = lhs
+  end function
+
+  pure function char_op(lhs, rhs) result(lhs_op_rhs)
+    character(len=1), intent(in)  :: lhs, rhs
+    character(len=1) :: lhs_op_rhs
+    lhs_op_rhs = min(lhs, rhs)
+  end function
+
+  pure function real_op(lhs, rhs) result(lhs_op_rhs)
+    real, intent(in) :: lhs, rhs
+    real :: lhs_op_rhs
+    lhs_op_rhs = lhs + rhs
+  end function
+
+  pure function double_precision_op(lhs, rhs) result(lhs_op_rhs)
+    integer, parameter :: double = kind(1.0D0)
+    real(double), intent(in) :: lhs, rhs
+    real(double) lhs_op_rhs
+    lhs_op_rhs = lhs + rhs
+  end function
+
+  pure function int_op(lhs, rhs) result(lhs_op_rhs)
+    integer, intent(in) :: lhs, rhs
+    integer :: lhs_op_rhs
+    lhs_op_rhs = lhs + rhs
+  end function
+
+  function not_pure(lhs, rhs) result(lhs_op_rhs)
+    integer, intent(in) :: lhs, rhs
+    integer :: lhs_op_rhs
+    lhs_op_rhs = lhs + rhs
+  end function
+
+  pure function too_many_args(lhs, rhs, foo) result(lhs_op_rhs)
+    integer, intent(in) :: lhs, rhs, foo
+    integer lhs_op_rhs
+    lhs_op_rhs = lhs + rhs
+  end function
+
+  pure function array_result(lhs, rhs)
+    integer, intent(in) :: lhs, rhs
+    integer array_result(1)
+    array_result = lhs + rhs
+  end function
+
+  pure function allocatable_result(lhs, rhs)
+    integer, intent(in) :: lhs, rhs
+    integer, allocatable :: allocatable_result
+    allocatable_result = lhs + rhs
+  end function
+
+  pure function pointer_result(lhs, rhs)
+    integer, intent(in) :: lhs, rhs
+    integer, pointer :: pointer_result
+    allocate(pointer_result, source=lhs + rhs )
+  end function
+
+  pure function array_args(lhs, rhs)
+    integer, intent(in) :: lhs(1), rhs(1)
+    integer array_args
+    array_args = lhs(1) + rhs(1)
+  end function
+
+  pure function allocatable_args(lhs, rhs) result(lhs_op_rhs)
+    integer, intent(in), allocatable :: lhs, rhs
+    integer lhs_op_rhs
+    lhs_op_rhs = lhs + rhs
+  end function
+
+  pure function pointer_args(lhs, rhs) result(lhs_op_rhs)
+    integer, intent(in), pointer :: lhs, rhs
+    integer lhs_op_rhs
+    lhs_op_rhs = lhs + rhs
+  end function
+
+  pure function polymorphic_args(lhs, rhs) result(lhs_op_rhs)
+    class(foo_t), intent(in) :: lhs, rhs
+    type(foo_t) lhs_op_rhs
+    lhs_op_rhs%n = lhs%n + rhs%n
+  end function
+
+  pure function optional_args(lhs, rhs) result(lhs_op_rhs)
+    integer, intent(in), optional :: lhs, rhs
+    integer lhs_op_rhs
+    if (present(lhs) .and. present(rhs)) then
+      lhs_op_rhs = lhs + rhs
+    else
+      lhs_op_rhs = 0
+    end if
+  end function
+
+  pure function target_mismatch(lhs, rhs) result(lhs_op_rhs)
+    integer, intent(in), target  :: lhs
+    integer, intent(in) :: rhs
+    integer lhs_op_rhs
+    lhs_op_rhs = lhs + rhs
+  end function
+
+  pure function value_mismatch(lhs, rhs) result(lhs_op_rhs)
+    integer, intent(in), value:: lhs
+    integer, intent(in) :: rhs
+    integer lhs_op_rhs
+    lhs_op_rhs = lhs + rhs
+  end function
+
+  pure function asynchronous_mismatch(lhs, rhs) result(lhs_op_rhs)
+    integer, intent(in), asynchronous:: lhs
+    integer, intent(in) :: rhs
+    integer lhs_op_rhs
+    lhs_op_rhs = lhs + rhs
+  end function
+
+end program