Restructure modules #68

ktras · 2024-02-27T00:26:09Z

Closes #64.

interface of `prif_init` into `prif` module and change `program_startup_s` to extend from `prif_private_s`.

`prif`. Change `program_termination_s` to extend `prif_private_s`. Update all `program_termination_m` use-stmts.

`atomic_s` to extend `prif_private_s`.

definition of `notify_type` to `prif_private_s`. Change `notify_s` to extend `prif_private_s`.

`events_s` to extend `prif_private_s`.

`critical_s` to extend `prif_private_s`.

`locks_s` to extend `prif_private_s`.

`synchronization_s` to extend `prif_private_s`.

the interfaces for the collectives to `prif` and keep the operation abstract interfaces in the newly named module. Change all the submodules with the definitions of the collectives to extend `prif_private_s`.

`prif_queries_s` to extend `prif_private_s`.

`image_queries_s` to extend `prif_private_s`.

`coarray_queries_s` to extend `prif_private_s`.

`alias_s` to extend `prif_private_s`.

`coarray_access_s` to extend `prif_private_s`.

`teams_m` and move all their interfaces to `prif`. Change `allocation_s` and `teams_s` to extend `prif_private_s`. Update all related `use-stmts`

bonachea

Overall this looks like the wonderful and long-overdue cleanup we discussed.

Added a few questions and minor requests.

bonachea · 2024-02-27T04:10:30Z

src/prif.f90

It's difficult to audit all the cross-file declaration motion in this PR.

Were there any meaningful changes to the contents of the public prif_ module subroutine interfaces as they moved from the *_m.f90 files to prif.f90?

I made no changes to the content of the interfaces when moving them. Just cut and paste from one module to the other. And I was as careful as I could be to ensure that no changes were accidentally made.

src/caffeine/prif_private_s.f90

src/prif.f90

src/caffeine/collective_helpers_m.f90

the types defined in the `prif` module, and move the Fortran intrinsic derived types to the `prif` module

ktras · 2024-02-28T00:12:43Z

I just moved the definition of handle_data to the bottom of the prif module. prif_coarray_handle and prif_team_type type definitions cannot be moved to the same location because there are arguments of those types above.

bonachea · 2024-02-28T02:45:49Z

I just moved the definition of handle_data to the bottom of the prif module. prif_coarray_handle and prif_team_type type definitions cannot be moved to the same location because there are arguments of those types above.

I thought we tried this in our prototyping meeting and decided that at least one compiler rejected the declaration of prif_coarray_handle when the handle_data pointer field was an incomplete type? What compilers have you successfully tested against?

Of course what ultimately matters most is what flang-new will accept, which I'm guessing is currently "none of this". On a related note, I think this PR needs to update the table in the top-level README where we track major modern Fortran features the Caffeine implementation requires. We at least need to add "Submodule support [1]" (which I believe was added in F08?), and probably remove the [1] from contiguous attribute, which is now "baked into" the PRIF API. There may be other updates needed.

bonachea

Thanks for the recent changes, which look good.

Some unresolved tasks remain.

types to a single place in `prif`

ktras · 2024-02-28T18:17:22Z

I thought we tried this in our prototyping meeting and decided that at least one compiler rejected the declaration of prif_coarray_handle when the handle_data pointer field was an incomplete type? What compilers have you successfully tested against?

When compiling the whole software stack, the only compiler we are currently developing with is gfortran.

Based on this comment, I tried compiling just the prif module (with the handle_data type definition at the bottom of the module) with the following compilers and with the following results:

Compiler	Result
gfortran	compiles
ifx	compiles
cray ftn	compiles
nvfortran	compiles
nagfor	doesn't compile for different reason (doesn't have `c_ptrdiff_t` support).

in calls to `prif_co_reduce`.

the last remaining location that uses them, `co_reduce_s`. Remove `collective_helpers_m`.

ktras · 2024-02-28T19:05:40Z

@bonachea Thank you for the further feedback. I have answered your compiler question above and then updated the PR in reference to your requested changes around collective_helpers_m and replied to that comment above. I believe I have addressed all of your review comments to this point. Please let me know about any further feedback, thanks!

bonachea

LGTM! Thanks for the overhaul and recent fixes.

bonachea · 2024-02-28T23:43:12Z

Wooops, I just noticed that doc-generator.md also needs updates as a result of this reorg, assuming we are still maintaining the FORD docs.

bonachea

After additional thought, I've added two further minor requests.

src/caffeine/prif_private_s.f90

redundant when defining PRIF around which procedures should provide termination.

everythingfunctional

LGTM, especially since the changes to the tests were so minimal.

bonachea

LGTM! Thanks for this valuable cleanup

ktras added 17 commits February 26, 2024 10:50

Begin module restructing. Create prif_private_s submodule. Move

1209e58

interface of `prif_init` into `prif` module and change `program_startup_s` to extend from `prif_private_s`.

Remove program_termination_m and move all its interfaces to

cd9b780

`prif`. Change `program_termination_s` to extend `prif_private_s`. Update all `program_termination_m` use-stmts.

Remove atomic_m and move all its interfaces to prif. Change

e1f8e4a

`atomic_s` to extend `prif_private_s`.

Remove notify_m and move all its interfaces to prif. Move

9873513

definition of `notify_type` to `prif_private_s`. Change `notify_s` to extend `prif_private_s`.

Remove events_m and move all its interfaces to prif. Change

9794aed

`events_s` to extend `prif_private_s`.

Remove critical_m and move all its interfaces to prif. Change

37725c3

`critical_s` to extend `prif_private_s`.

Remove the critical_m module that is no longer used.

b0e54a8

Remove locks_m and move all its interfaces to prif. Change

a301b8f

`locks_s` to extend `prif_private_s`.

Remove synchronization_m and move all its interfaces to prif. Change

e5f357b

`synchronization_s` to extend `prif_private_s`.

Rename collective_subroutines_m to collective_helpers_m. Move

4f77f71

the interfaces for the collectives to `prif` and keep the operation abstract interfaces in the newly named module. Change all the submodules with the definitions of the collectives to extend `prif_private_s`.

Remove prif_queries_m and move all its interfaces to prif. Change

858b1f1

`prif_queries_s` to extend `prif_private_s`.

Remove image_queries_m and move all its interfaces to prif. Change

8342585

`image_queries_s` to extend `prif_private_s`.

Remove coarray_queries_m and move all its interfaces to prif. Change

6bbb607

`coarray_queries_s` to extend `prif_private_s`.

Remove alias_m and move all its interfaces to prif. Change

f5b13e4

`alias_s` to extend `prif_private_s`.

Remove coarray_access_m and move all its interfaces to prif. Change

df5fee0

`coarray_access_s` to extend `prif_private_s`.

Finish module restructuring by removing allocation_m and

989f629

`teams_m` and move all their interfaces to `prif`. Change `allocation_s` and `teams_s` to extend `prif_private_s`. Update all related `use-stmts`

Make all components of derived types defined in prif private.

542ffd1

ktras requested review from bonachea and everythingfunctional February 27, 2024 00:26

Fix order of an interface in prif

b154abe

bonachea requested changes Feb 27, 2024

View reviewed changes

ktras added 2 commits February 27, 2024 15:50

Add explicit private and public attributes markers to each of

10d70ef

the types defined in the `prif` module, and move the Fortran intrinsic derived types to the `prif` module

Move private type definition to bottom of prif

23fcb6e

ktras requested a review from bonachea February 28, 2024 00:12

bonachea requested changes Feb 28, 2024

View reviewed changes

Restrict public and private attribute declarations for the

fe4ca67

types to a single place in `prif`

Update README prerequisite table.

f5e0cf3

ktras added 2 commits February 28, 2024 10:45

Update caf_co_reduce_test to only use contained functions

a95d8f4

in calls to `prif_co_reduce`.

Move all abstract interfaces from collective_helpers_m into

80459f5

the last remaining location that uses them, `co_reduce_s`. Remove `collective_helpers_m`.

ktras requested a review from bonachea February 28, 2024 19:03

bonachea approved these changes Feb 28, 2024

View reviewed changes

bonachea requested changes Feb 29, 2024

View reviewed changes

src/caffeine/prif_private_s.f90 Show resolved Hide resolved

Update doc-generator.md and delete a module and submodule made

bd8e501

redundant when defining PRIF around which procedures should provide termination.

ktras requested a review from bonachea March 4, 2024 21:03

everythingfunctional approved these changes Mar 4, 2024

View reviewed changes

bonachea approved these changes Mar 5, 2024

View reviewed changes

ktras merged commit 01ccb44 into main Mar 5, 2024
6 checks passed

ktras deleted the restructure-modules branch March 5, 2024 00:37

bonachea mentioned this pull request Mar 15, 2024

Add Caffeine's own custom assert subroutine #76

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restructure modules #68

Restructure modules #68

ktras commented Feb 27, 2024

bonachea left a comment

bonachea Feb 27, 2024

ktras Feb 27, 2024 •

edited

Loading

ktras commented Feb 28, 2024

bonachea commented Feb 28, 2024

bonachea left a comment

ktras commented Feb 28, 2024

ktras commented Feb 28, 2024

bonachea left a comment

bonachea commented Feb 28, 2024

bonachea left a comment

everythingfunctional left a comment

bonachea left a comment

Restructure modules #68

Restructure modules #68

Conversation

ktras commented Feb 27, 2024

bonachea left a comment

Choose a reason for hiding this comment

bonachea Feb 27, 2024

Choose a reason for hiding this comment

ktras Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

ktras commented Feb 28, 2024

bonachea commented Feb 28, 2024

bonachea left a comment

Choose a reason for hiding this comment

ktras commented Feb 28, 2024

ktras commented Feb 28, 2024

bonachea left a comment

Choose a reason for hiding this comment

bonachea commented Feb 28, 2024

bonachea left a comment

Choose a reason for hiding this comment

everythingfunctional left a comment

Choose a reason for hiding this comment

bonachea left a comment

Choose a reason for hiding this comment

ktras Feb 27, 2024 •

edited

Loading