Skip to content
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.

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

Field gang #16

Merged
merged 45 commits into from
Jan 15, 2024
Merged

Field gang #16

merged 45 commits into from
Jan 15, 2024

Conversation

pmarguinaud
Copy link
Collaborator

@pmarguinaud pmarguinaud commented Dec 5, 2023

This branch implements two new classes designed to handle multi-parameter fields such as aerosols. Its purpose is to provide alternative data representation for such fields in the dynamics and in the physics. This is best described in the following presentation:

fieldbuffer.pdf

The two new classes handle :

  • existing fields (eg GMV/GFL) : eg FIELD_3RB_GANG_WRAPPER
  • standalone fields (typically temporaries) : eg FIELD_3RB_GANG_OWNER

The implementation relies on a parent/children relationship between a master field of dimension N and children fields of dimensions N-1. All children fields delegate the management of DEVPTR to their parent; the STATUS of the children is forced to be coherent with the status of the parent.

Other minor cleaning/features are included:

  • split some (big) files into smaller ones
  • move constants into a separate module
  • allow the user to replace ABOR1 by another routine + wrap ABOR1 in FIELD_ABORT

…ld_RANKSUFF_gather_module.fypp & field_gathscat_type_module.fypp
CMakeLists.txt Outdated

## preprocess fypp files
foreach (SUFF IN ITEMS IM RM RB RD LM)
string (TOLOWER ${SUFF} suff)
foreach (RANK RANGE 2 5)
foreach (FUNC IN ITEMS "" _gathscat _access _util _array_util)
add_custom_command (OUTPUT field_${RANK}${suff}${FUNC}_module.F90
COMMAND ${FYPP} -n -DRANK=${RANK} -DSUFF='${SUFF}' -m os -M ${CMAKE_CURRENT_SOURCE_DIR} -m fieldType
Copy link
Collaborator

Choose a reason for hiding this comment

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

Were the fypp line annotations removed intentionally? (the -n arg)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes and no. Much easier to look at real Fortran code when debugging, but we can got back to fypp line indexing if you do not like it.

! Mark data dirty on the device: the pointer returned by GET_VIEW may be rw
IF (IAND (SELF%ISTATUS, NDEVFRESH)==NDEVFRESH) THEN
SELF%ISTATUS = IAND(SELF%ISTATUS, NOT(NDEVFRESH))
CALL SELF%SET_DEVICE_DIRTY ()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I could be mistaken, but I think here we do not mark the children as dirty on the device if GET_VIEW is called on the gang. I think the children should also be marked as dirty.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The code has moved, but I think SET_DEVICE_DIRTY will call SET_STATUS, which will also update children status.


ELSE

SELECT CASE (IRAND (1, 134))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure I understand the use of IRAND here. Are we selecting the case to test based on an RNG, or am I missing something here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, we generate random (but reproducible) sequences of operations on the 4D fields and its 3D children. Note it is not feasible to explore all the combinations.

Copy link
Collaborator

Choose a reason for hiding this comment

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

For the record, I don't think RNG-based testing is a good idea, and I still don't fully understand the need for this here. However, fixing this probably requires a rethink of the test base implementation, which is beyond this PR.

Copy link
Collaborator

@awnawab awnawab left a comment

Choose a reason for hiding this comment

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

Thanks a lot @pmarguinaud for your implementation of the FieldGang, along with all the other useful additions! Thanks also for adding the missing CUDAHOSTUNREGISTER. Once the suggested ifdef guards for that have been added, I am happy to merge this contribution.

DATA = C_LOC (HST (${ ', '.join (map (lambda i: 'LBOUND (HST, ' + str (i) + ')', range (1, ft.rank+1))) }$))

IF (PINNED) THEN
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
IF (PINNED) THEN
#:if defined('CUDA')
IF (PINNED) THEN

IF (ISTAT /= 0) THEN
CALL FIELD_ABORT ("${ft.name}$_OWNER: FAILED TO UNREGISTER PAGE-LOCKED MEMORY")
ENDIF
ENDIF
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
ENDIF
ENDIF
#:endif

Copy link
Collaborator

@mlange05 mlange05 left a comment

Choose a reason for hiding this comment

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

Form a high level this seems ok, but the size of the PR makes a meaningful detailed review very hard. Nevertheless, a good feature to have, so tentatively GTG from me.


ELSE

SELECT CASE (IRAND (1, 134))
Copy link
Collaborator

Choose a reason for hiding this comment

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

For the record, I don't think RNG-based testing is a good idea, and I still don't fully understand the need for this here. However, fixing this probably requires a rethink of the test base implementation, which is beyond this PR.

@awnawab awnawab merged commit a604008 into ecmwf-ifs:main Jan 15, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants