Page MenuHomePhabricator

[flang] Add a semantics test for co_reduce
ClosedPublic

Authored by rouson on Nov 2 2021, 11:10 PM.

Details

Summary

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

Diff Detail

Event Timeline

rouson created this revision.Nov 2 2021, 11:10 PM
Herald added a project: Restricted Project. · View Herald Transcript
rouson requested review of this revision.Nov 2 2021, 11:10 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 2 2021, 11:10 PM
Herald added a subscriber: jdoerfert. · View Herald Transcript
rouson updated this revision to Diff 384898.EditedNov 4 2021, 4:05 PM
  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.

@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 added a subscriber: ekieri.EditedNov 18 2021, 12:52 AM

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

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 added inline comments.
flang/test/Semantics/collectives05.f90
5

What is the evaluation stage?

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 added inline comments.Nov 21 2021, 12:55 PM
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 added a comment.EditedNov 30 2021, 2:37 PM

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 added inline comments.Nov 30 2021, 2:47 PM
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 added a comment.Dec 1 2021, 1:20 PM

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 added a comment.EditedDec 1 2021, 3:28 PM

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 updated this revision to Diff 391176.Dec 1 2021, 5:49 PM

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 added a comment.Dec 5 2021, 1:18 PM

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 this revision to Diff 392985.Dec 8 2021, 5:01 PM

updated patch based on comments from @ekieri

rouson added a comment.Dec 8 2021, 5:08 PM

@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 updated this revision to Diff 393238.Dec 9 2021, 11:44 AM

The latest version of the patch addresses all reviewer comments.

ekieri added a comment.Dec 9 2021, 2:05 PM

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 accepted this revision.Dec 13 2021, 1:51 PM

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
This revision is now accepted and ready to land.Dec 13 2021, 1:51 PM
rouson marked an inline comment as done.Dec 16 2021, 11:56 AM
rouson added inline comments.
flang/test/Semantics/collectives05.f90
95

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

ktras added inline comments.Dec 16 2021, 12:04 PM
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 updated this revision to Diff 394959.Dec 16 2021, 12:27 PM
rouson marked an inline comment as done.

Handled all review comments:

  1. Fixed two grammatical errors in comments.
  2. Clarified the To Do comment.
rouson marked 6 inline comments as done.Dec 16 2021, 12:27 PM
This revision was automatically updated to reflect the committed changes.