Skip to content

Improve teams implementation #794

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Improve teams implementation #794

wants to merge 10 commits into from

Conversation

vehre
Copy link
Collaborator

@vehre vehre commented Apr 10, 2025 •
edited
Loading

Improve the implemenation of team's functions to adhere to the Fortran 2018 standard and gfortran from 15 on.

coverage on master
Codecov branch

Summary of changes

Summarize what you changed

Rationale for changes

OpenCoarrays was not implementing teams support correctly. This pull request adds the missing pieces and reworks non-standard complying parts. It also fixes the coarrays allocated in a teams block need to be freeed on exit issue.

Fixes #779
Fixes #545

Additional info and certifications

This pull request (PR) is a:

  • Bug fix
  • Feature addition
  • Other, Please describe:

I certify that

  • I certify that:
    • I have reviewed and followed the contributing guidelines
    • I will wait at least 24 hours before self-approving the PR to give another
      OpenCoarrays developer a chance to review my proposed code
    • I have not introduced errant white space (no trailing white space or white space errors may
      be introduced)
    • I have added an explanation of what these changes do and why they should be included
    • I have checked to ensure there aren't other open Pull Requests for the same change
    • I have you written new tests for these changes
    • I have successfully tested these changes locally
    • I have commented any non-trivial, non-obvious code changes
    • The commits are logically atomic, self consistent and coherent
    • The commit messages follow best practices
    • Test coverage is maintained or increased after this is merged

Code coverage data

coverage on master

@vehre vehre requested review from zbeekman and rouson April 10, 2025 13:44
Base automatically changed from vehre/issue-774-no-caf-aware-allocatable to main April 10, 2025 19:04
zbeekman
zbeekman reviewed Apr 10, 2025
View reviewed changes
CMakeLists.txt Outdated
add_caf_test(teams_this_image 8 teams_this_image)
add_caf_test(teams_num_images 8 teams_num_images)
else()
add_caf_test(team_number_pre15 8 team_number_pre15)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests for team_number are essentially the same, just with an additional assert that was commented out included. I do not like the idea of maintaining two separate files with practically the exact same content, nor do I like having to include this else clause in the CMake file. @vehre would you be able to just give the test file a capital .F90 file extension and use a preprocessor macro to conditionally include the assertion in the test?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did that. Sorry, for messing that up. On gfortran the policy is to create a new test. So I did it here, too. I've now added the preprocessor directives.

zbeekman
zbeekman reviewed Apr 10, 2025
View reviewed changes
src/tests/unit/teams/sync-team.f90 Outdated
@@ -24,6 +25,7 @@ program main
end if
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test isn't really actually testing the functionality of form team and sync team, it's more of a smoke test. It would be nice to add some assertions using get_team and team_number to verify this is functioning as expected.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, some check on the synchornization. I also do not understand the point of the second set of calls to:

         sync team(team(CHILD_TEAM))
         sync team(team(CURRENT_TEAM))

Shouldn't calling each one once be sufficient?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I don't know what the initial author had in mind with that. I can only speculate, that he encountered some deadlocks when waiting for different teams.

I have reverted my changes to the sync_team.f90 and added two new tests with hopefully more "beef". Thank you for making me do this, because test_teams_1.f90 had me fix some errors in gfortran and in OpenCoarrays. Therefore there is a longer commit list now.

zbeekman
zbeekman reviewed Apr 10, 2025
View reviewed changes
src/tests/unit/teams/teams_num_images.f90
if (j2 /= num / 2) stop 2

j3 = num_images(MOD(me, 2) + 1)
if (j3 /= num / 2) stop 3
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can follow this, but a comment describing what each test is actually testing would help clarify things.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

zbeekman
zbeekman requested changes Apr 10, 2025
View reviewed changes
Copy link
Collaborator

@zbeekman zbeekman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vehre in general this looks good. Please address or respond to individual comments. Also, in the tests there are a lot of somewhat complicated integer arithmetic expressions. Often these are for determining the team. They seem like magic numbers unless you really take the time to go through them. It would be helpful to include a comment describing how you arrived at the expression (or more details on what the expression is).

@vehre vehre mentioned this pull request Apr 14, 2025
Closed
1 task
@vehre vehre force-pushed the vehre/issue-779-form-team branch from ae7d58c to 8fe8625 Compare April 17, 2025 13:35
@vehre
Copy link
Collaborator Author

vehre commented Apr 17, 2025

@vehre in general this looks good. Please address or respond to individual comments. Also, in the tests there are a lot of somewhat complicated integer arithmetic expressions. Often these are for determining the team. They seem like magic numbers unless you really take the time to go through them. It would be helpful to include a comment describing how you arrived at the expression (or more details on what the expression is).

Er, most of the computations are about doing odd-even team splits or the like. Can you point me to a specific one?

vehre added 10 commits April 28, 2025 08:59
@vehre
Improve teams implementation
168615a 
Improve the implementation of team's functions to adhere to the Fortran
2018 standard and gfortran from 16 on.
@vehre
Fix team_number test.
e64aa38
@vehre
Remove wrong error.
3dcbf4b 
Issuing this error was a misinterpretation of the standard.
@vehre
Enhance debug logging.
79af6ff
@vehre
Add owning memory flag.
e2cba54
@vehre
Keep team_translate rework.
82fbd4b
@vehre
Use COMM_TEAM instead of COMM_WORLD.
58fef7f 
Revert changes to CAF_COMM_WORLD and introduce CAF_COMM_TEAM.
Let all communication happen on CAF_COMM_WORLD and map the
image indexes accordingly. Use CAF_COMM_TEAM for all collective
ops.
@vehre
Add more "beefy" tests.
848e023 
Add tests that really make use of teams and not only a smoke test.
@vehre
Add comments to teams_num_images test.
680df81
@vehre
Fixes for use with gcc<16.
5661643
@vehre vehre force-pushed the vehre/issue-779-form-team branch from f3018bb to 5661643 Compare April 28, 2025 11:25
@vehre
Copy link
Collaborator Author

vehre commented Apr 28, 2025

@zbeekman I had to adapt the teams for gcc-16, because the merge reQuest on gfortran did not make it in time for the gcc-15 release. I.e. improved teams support will now be on gcc-16 only, if no one reQuests a backport.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zbeekman zbeekman zbeekman requested changes

@rouson rouson Awaiting requested review from rouson

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

RFE: FORM TEAM statement Defect: get_team() does not compile properly
2 participants
@vehre @zbeekman