Page MenuHomePhabricator

[flang] Add a semantics test for co_sum
ClosedPublic

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

Details

Summary

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

Diff Detail

Event Timeline

rouson created this revision.Nov 2 2021, 7:11 PM
Herald added a project: Restricted Project. · View Herald Transcript
rouson requested review of this revision.Nov 2 2021, 7:11 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 2 2021, 7:11 PM
Herald added a subscriber: jdoerfert. · View Herald Transcript
kiranchandramohan requested changes to this revision.Nov 3 2021, 11:13 AM
kiranchandramohan added inline comments.
flang/test/Semantics/collectives01.f90
2

Thanks @rouson for these tests. Recently we switched to using python (test_errors.py) instead of shell scripts (test_errors.sh) so that these tests run on Windows as well.

Please see some of the other tests for an example. (https://github.com/llvm/llvm-project/blob/main/flang/test/Semantics/critical01.f90)

Please switch to using that otherwise, the tests will fail.

This revision now requires changes to proceed.Nov 3 2021, 11:13 AM
ktras added a comment.Nov 3 2021, 2:09 PM

I have locally implemented co_sum and was able to get this test to pass if these two error messages are changed, and if the python program is added, as suggested by @kiranchandramohan. Is there a way to make this test expectedly fail to get it into the repository? And then I can submit a patch with my implementation of co_sum in flang/lib/Evaluate/intrinsics.cpp?

flang/test/Semantics/collectives01.f90
37

The expected error will be too many actual arguments for intrinsic 'co_sum'

ktras added inline comments.Nov 3 2021, 2:12 PM
flang/test/Semantics/collectives01.f90
33

Please change the marks to ' marks.

I have locally implemented co_sum and was able to get this test to pass if these two error messages are changed, and if the python program is added, as suggested by @kiranchandramohan. Is there a way to make this test expectedly fail to get it into the repository? And then I can submit a patch with my implementation of co_sum in flang/lib/Evaluate/intrinsics.cpp?

You can use XFAIL to mark the test as expected to fail.
See https://github.com/llvm/llvm-project/blob/main/flang/test/Semantics/omp-sections03.f90 for an example.
https://llvm.org/docs/TestingGuide.html#constraining-test-execution

ktras added a comment.Nov 4 2021, 10:32 AM

Thank you @kiranchandramohan. I was trying to find that exact info, thanks for helping point me in the right direction!

rouson updated this revision to Diff 384914.Nov 4 2021, 5:11 PM
  1. Add co_sum 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 marked 3 inline comments as done.Nov 4 2021, 5:13 PM

Made update based on comments.

ktras accepted this revision.Nov 4 2021, 5:39 PM

Thanks for making the changes. LGTM.

kiranchandramohan accepted this revision.Nov 5 2021, 3:02 PM

LGTM. Thanks for this testcase.

This revision is now accepted and ready to land.Nov 5 2021, 3:02 PM

@rouson @ktras Do you need help to submit this patch?

ktras added a comment.Nov 10 2021, 9:27 AM

@rouson @ktras Do you need help to submit this patch?

Yes, I think we do. I don't have commit privileges and I don't think @rouson does either.

I think it's best for you to do the commit yourself. Here’s how.

The first step is to get committer access to the llvm-project repository. You can request commit access for the llvm-project here: https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access. Once access is granted, an invitation should be visible here: https://github.com/llvm.

Once you have committer access (actually, you can do this now), you should incorporate your changes into the latest source code from llvm-project, and make sure that everything builds and tests correctly. Here’s how to do that:

Start in your private repository in the branch that contains your changes.
If you have multiple commits, run git rebase -i to squash them into a single commit.
Merge the latest changes from llvm-project into your branch:

• git checkout main
• git pull
• git checkout mybranch
• git rebase main

Rebuild and retest to verify that your changes still work.
Push your changes to the main branch in the llvm-repository: git push origin mybranch:main
This revision was automatically updated to reflect the committed changes.