Improvement of delayed allocation mechanism #105

dareg · 2025-08-01T13:38:08Z

In the current field api version, the delayed allocation mechanism is very basic. One can ask to delay the allocation of the pointer to the CPU data and the pointer to the GPU data but the two are still allocated at the same time.
With this PR the delayed allocation mechanism is improved, and the the allocations of the CPU pointer and the GPU pointer are entirely separated. If the data is never accessed on CPU, then the CPU pointer is never allocated, same on the GPU side.

Main changes:

delayed mechanism rewritten
new way of managing field api status. No more get_status with bit comparison. Can now use proper add_status, remove_status, and is_status subroutines
store lbound and ubound in wrapper field (was necessary and simplify the code since it reduce the difference between wrapper and owner)
get_view is now callable even when the data are not on CPU or not allocated. They will be moved or allocated if necessary by get_view
update NVHPC compiler version used in CI because of a bug in test_gang. Was 23.5, is now updated to 24.5, first version to compile it properly.

…ne was

…ependently

It's necessary to call set_device_dirty in get_view, because when called from a child of a parent (in a gang), the child will call the overloaded set_device_dirty which also call set_device_dirty on the parent.

Before this modification, the delayed option let the user allocate the data on CPU and GPU after the call of field_new, but the two were still happening at the same time. With this modification, the allocation on CPU and the allocation on GPU are separated. To do so, the satus system had to be slighty modified since the whole library used to assume that the two data pointers were allocated at the same time. Finally the usege of the status system has been simplified with the addition of three helper functions (add, remove, is) that let us modify or query the status without having to do bits comparisons in the middle of the code.

The checks in these test-cases assumed the data pointers werea allocated in the field_new, but with implicit delayed it is not necessary true. Add a call to get_host_data to force the allocation of the pointers.

This subroutine was testing that the program did not work when calling get_view on data recently updated on GPU. But get_view is now able to handle this case.

awnawab

Many thanks @dareg for this contribution 🙏 Most of the changes look fine to me, and some things like the more explicit ADD_STATUS/REMOVE_STATUS are big and very welcome improvements. I've left a few minor comments, mostly about redundant constants or code paths now being removed.

The only major disagreement I have is about the changes to %GET_VIEW. The abort there is an incredibly useful safety mechanism, and being forced to synchronise explicitly before calls to %GET_VIEW has saved us from many many painful debugging sessions. In a codebase as complex as the IFS, I think enforcing explicit synchronisation is a better, safer and simpler design choice.

As far as I understand your work, reverting the changes to %GET_VIEW shouldn't affect any of the new delayed allocation functionality, and you would just have to do the synchronisation explicitly before calling %GET_VIEW.

awnawab · 2025-08-04T14:40:08Z

src/core/field_RANKSUFF_module.fypp

@@ -277,7 +271,7 @@ CONTAINS
      SELF%UBOUNDS(${ft.rank}$) = OML_MAX_THREADS ()
    ENDIF

-    CALL SELF%SET_STATUS (UNALLOCATED)
+    CALL SELF%SET_STATUS (UNINITIALIZED)


Does this mean the UNALLOCATED status is now defunct? If so, we should delete it and all the other constants can be shifted right by 1.

awnawab · 2025-08-04T14:51:30Z

src/core/field_RANKSUFF_module.fypp

-      CALL FIELD_ABORT ("GET_VIEW WAS CALLED, BUT DATA IS NOT PRESENT ON HOST")
+    ${ft.type}$, POINTER :: ZPTR(${ft.shape}$)
+
+!When the first thread reach this line it checks if the data are available


%GET_VIEW calling abort has been a super useful safety feature, and it's often highlighted examples where we've forgotten to synchronise a field. The abort in %GET_VIEW has also helped us catch several OpenMP firstprivatization errors, which are notoriously hard to track down.

I think an explicit API is always better than an implicit API, and rather than %GET_VIEW implicitly synchronising or allocating the field, we should force the user to explicitly SYNCHRONISE. With this kind of implicit synchronisation, data transfers and allocations become much harder to track. Based on the above, I'm not in favour of the changes to %GET_VIEW.

awnawab · 2025-08-04T15:05:49Z

src/core/field_RANKSUFF_module.fypp

@@ -483,12 +491,14 @@ $:offload_macros.detach(ptr=['SELF%DEVPTR'], indent=6)
    ${ft.type}$, POINTER,          INTENT(INOUT) :: PTR(${ft.shape}$)
    INTEGER (KIND=JPIM), OPTIONAL, INTENT(IN)    :: QUEUE

-    IF(SELF%GET_STATUS ()==UNALLOCATED)THEN
+    IF(.NOT. ASSOCIATED(SELF%PTR))THEN


This confirms doesn't it that the UNALLOCATED status is now redundant and can be removed?

awnawab · 2025-08-04T15:24:52Z

src/core/field_RANKSUFF_module.fypp

      CALL SELF%COPY_DATA (ND2H, QUEUE)
-      CALL SELF%SET_STATUS (IOR (SELF%GET_STATUS (), NHSTFRESH))
+      CALL SELF%ADD_STATUS (NHSTFRESH)
+      CALL SELF%REMOVE_STATUS (UNINITIALIZED_CPU)


The removal of the UNINITIALIZED_CPU status should be unconditional, since we are always returning a pointer to access the host data.

awnawab · 2025-08-04T15:28:53Z

src/core/field_RANKSUFF_module.fypp

@@ -533,7 +543,8 @@ $:offload_macros.detach(ptr=['SELF%DEVPTR'], indent=6)
  SUBROUTINE ${ftn}$_CREATE_DEVICE_DATA (SELF)
    CLASS(${ftn}$) :: SELF

-    CALL DEV_ALLOCATE_HST (DEV=SELF%DEVPTR, HST=SELF%PTR, MAP_DEVPTR=SELF%MAP_DEVPTR)
+    CALL DEV_ALLOCATE_DIM (DEV=SELF%DEVPTR, LBOUNDS=SELF%LBOUNDS, UBOUNDS=SELF%UBOUNDS, MAP_DEVPTR=SELF%MAP_DEVPTR)


DEV_ALLOCATE_HST is now redundant and should therefore be removed.

awnawab · 2025-08-04T15:31:16Z

src/core/field_RANKSUFF_module.fypp

    ENDIF
    PTR (${ft.lbptr}$) => SELF%DEVPTR (${','.join(':' for _ in range(ft.rank))}$)
    IF (IAND (MODE, NWR) /= 0) THEN
-      CALL SELF%SET_STATUS (IAND (SELF%GET_STATUS (), NOT (NHSTFRESH)))
+      CALL SELF%REMOVE_STATUS (NHSTFRESH)


I do really like the more explicit ADD_STATUS and REMOVE_STATUS calls, much more intuitive and readable than the binary operators. Nice!

awnawab · 2025-08-04T15:42:13Z

python_utils/offload_backends/nvhpc/openmp.py

+    @classmethod
+    def reinit_gpu_context(cls):
+        """
+        Used to force reinitialization of GPU device.


Really appreciate the detailed documentation 🙏

awnawab · 2025-08-04T15:50:20Z

tests/test_host_mem_pool.F90


        POOL_BLOCK_SIZE = 1024_INT64*SIZEOF(REAL(1., KIND=JPRD))

        !... Check creation of memory pool
        CALL FIELD_NEW(F1, UBOUNDS=[256], PERSISTENT=.TRUE., POOLED=.TRUE.)
+        CALL F1%GET_HOST_DATA_RDWR(PTR1)


Is this really necessary? Since the default behaviour is to allocate, the host-side allocation should be performed regardless of the call to %GET_HOST_DATA_RDWR?

dareg · 2025-08-05T09:03:23Z

Hello @awnawab, thanks for your feedbacks. I will address them in the next days.

Regarding the new get_view behaviour, Philippe would really like to keep it. So would it be fine for you if we keep the new behaviour, but make it optional? By default get_view will abort when the data are not on CPU.

awnawab · 2025-08-05T09:06:43Z

Hi @dareg. Sure that works for me, as long as we keep the current behaviour as the default. Thanks!

It helps to keep the test-case functional whatever the option is.

awnawab · 2025-08-08T13:42:55Z

src/core/field_RANKSUFF_module.fypp

+! - if GET_VIEW_ABORT is TRUE then GET_VIEW will abort the program
+! - otherwise it then reachs a critical area, and check again in case another
+!   thread has moved them meanwhile. And if they are still not, then they are moved.
+    IF(.NOT. ASSOCIATED(SELF%PTR) &


A wrapped 0-sized field can also return .FALSE. for this association test, and we can't tell such a field apart from a delayed allocation owned field. In this case we'll wrongly call FIELD_ABORT. We have quite a few wrappers around 0-sized fields, and we should be able to safely call GET_VIEW on them, as typically the block dimension is not the one with zero size.

The cleanest workaround I can think of is to reinstate an UNALLOCATED status, and mark a delayed allocation field as such. Then the condition here would be SELF%IS_STATUS(UNALLOCATED) .OR. (SELF%IS_STATUS(NDEVFRESH) .AND. .NOT. SELF%IS_STATUS(NHSTFRESH)). All the checks in the new delayed allocation logic should then be based on the UNALLOCATED status rather than checking the association status of SELF%PTR.

dareg added 30 commits June 6, 2025 15:57

Add lbounds and ubounds to wrapper

ed7f067

Always use dim to allocate on device

e2bd04b

Don't create host data when calling get_device_data

62b43a3

set status correctly

50564d1

update get_host for when the host ptr was not allocated but the gpu o…

9cf8191

…ne was

force delayed allocation for owner

df7a8f5

Rework the status system to let the host and dev ptr be allocated ind…

5968c54

…ependently

Fix broken status modification

3b2116b

Fix broken get_view

82cdb3f

Fix transfer CPU<->GPU conditions

9c374a3

Fix get_view* test-cases

bda3c26

make sure data are allocated on cpu for get_view

e3dc484

delayed default value can now be controlled through a global

7c7ebfb

cleaning

184b500

cleaning

5f33c3b

cleaning

216601f

cleaning

bbc07a2

Add missing add_status and remove_status to field gang objects

7301193

Fix test-cases

09753cf

It's necessary to call set_device_dirty in get_view, because when called from a child of a parent (in a gang), the child will call the overloaded set_device_dirty which also call set_device_dirty on the parent.

Add more tests

c352310

Correct initialization when init_value is set

4ec4760

cleaning

5dcd5b5

Use correct negation function

28e69a9

Use correct argument name

8d8f013

There must be no return statement in main

2155321

cleaning

40b82f8

Use delayed by default

03a0246

Fix test-case

5633c1d

The checks in these test-cases assumed the data pointers werea allocated in the field_new, but with implicit delayed it is not necessary true. Add a call to get_host_data to force the allocation of the pointers.

Alloc in get view when necessary

0e6b0c8

dareg added 12 commits July 29, 2025 12:48

Test get view when not using GPU number 0

a4aa7c6

Recreate context for the current GPU, not necessary the 0th one

73b7cc6

Fix test-case when not using openacc

64131a6

Don't use delayed by default

afb0b9f

Update doc

4224cfa

Merge branch 'new_delayed' into new_delayed_prepare_pr

613e211

Remove now useless part of test-case

5df1113

This subroutine was testing that the program did not work when calling get_view on data recently updated on GPU. But get_view is now able to handle this case.

Cleaning

b85d5bc

Merge branch 'new_delayed' into new_delayed_prepare_pr

74d5b3e

Make it work with OpenMP GPU offloading

2d99770

Update NVHPC compiler version used in CI

195b7f2

Update doc

feb48a8

dareg requested review from pmarguinaud and awnawab August 1, 2025 13:38

github-actions bot added the contributor label Aug 1, 2025

Minor doc fix

648e7bf

awnawab added the approved-for-ci Approved to run hpc-ci label Aug 4, 2025

Merge branch 'main' into new_delayed_prepare_pr

1fb9d2b

github-actions bot removed the approved-for-ci Approved to run hpc-ci label Aug 4, 2025

awnawab added the approved-for-ci Approved to run hpc-ci label Aug 4, 2025

awnawab requested changes Aug 4, 2025

View reviewed changes

Remove now useless UNALLOCATED constant

1b0209a

github-actions bot removed the approved-for-ci Approved to run hpc-ci label Aug 6, 2025

dareg added 3 commits August 6, 2025 11:22

Remove now unused DEV_ALLOCATE_HST subroutine

d1e2922

Add an option to change get_view behaviour

b5a14ab

Add a compile option to switch behaviour of get_view

d0f9d01

It helps to keep the test-case functional whatever the option is.

awnawab added the approved-for-ci Approved to run hpc-ci label Aug 7, 2025

awnawab reviewed Aug 8, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Improvement of delayed allocation mechanism #105

Improvement of delayed allocation mechanism #105

Uh oh!

dareg commented Aug 1, 2025

Uh oh!

awnawab left a comment

Uh oh!

awnawab Aug 4, 2025

Uh oh!

awnawab Aug 4, 2025

Uh oh!

awnawab Aug 4, 2025

Uh oh!

awnawab Aug 4, 2025

Uh oh!

awnawab Aug 4, 2025

Uh oh!

awnawab Aug 4, 2025

Uh oh!

awnawab Aug 4, 2025

Uh oh!

awnawab Aug 4, 2025

Uh oh!

dareg commented Aug 5, 2025

Uh oh!

awnawab commented Aug 5, 2025

Uh oh!

awnawab Aug 8, 2025

Uh oh!

awnawab Aug 8, 2025

Uh oh!

Uh oh!

Improvement of delayed allocation mechanism #105

Are you sure you want to change the base?

Improvement of delayed allocation mechanism #105

Uh oh!

Conversation

dareg commented Aug 1, 2025

Uh oh!

awnawab left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dareg commented Aug 5, 2025

Uh oh!

awnawab commented Aug 5, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!