Add team features to native-multi-image.F90 app #269
Conversation
|
@bonachea I have not used macros for each individual team feature. In my initial draft of adding macros to my changes to this program, I did use macros for each individual team feature. However, particularly because of |
bonachea
left a comment
bonachea
left a comment
There was a problem hiding this comment.
Thanks for this PR. The overall design looks good, I added a few requests for improvements based on a quick skim
| res = get_team(CURRENT_TEAM) | ||
| res = get_team(INITIAL_TEAM) | ||
| res = get_team() | ||
| write(*,'(A,I3)') "Initial team number is ", team_number() |
There was a problem hiding this comment.
Please preserve the convention used in this file of upper-case for intrinsics (to help them stand-out relative to lower-case variable/procedure names defined in the program)
| res = get_team(CURRENT_TEAM) | |
| res = get_team(INITIAL_TEAM) | |
| res = get_team() | |
| write(*,'(A,I3)') "Initial team number is ", team_number() | |
| res = GET_TEAM(CURRENT_TEAM) | |
| res = GET_TEAM(INITIAL_TEAM) | |
| res = GET_TEAM() | |
| write(*,'(A,I3)') "Initial team number is ", TEAM_NUMBER() |
and similarly below
| if (ni < 2) then | ||
| if (me == 1) write(*,'(A)') "Please run program again with at least 2 images to test more TEAM features." | ||
| else |
There was a problem hiding this comment.
Why are we excluding single-image execution here?
By my reading the code below should still be fully correct (and hence testable) with a single image. It's obviously less interesting with only one image, but I don't see a reason to prohibit execution.
| integer :: me, ni, peer, tmp | ||
| integer :: me, ni, peer, tmp, team_id | ||
| character(len=5) :: c | ||
| type(team_type) :: subteam, res |
There was a problem hiding this comment.
This declaration should be conditional on HAVE_TEAM
|
Rebasing to pick-up CI changes |
bonachea
force-pushed
the
update-multi-image-app
branch
from
7f5f0d2 to
b9a42b3
Compare
bonachea
left a comment
bonachea
left a comment
There was a problem hiding this comment.
Recording one new observation
bonachea
force-pushed
the
update-multi-image-app
branch
from
b9a42b3 to
3c38a05
Compare
|
Rebasing to pick-up CI changes |
2 participants
This PR will not pass CI (with the current macros being set for flang) until PR #165573 is merged into the llvm-project repo.