Add UNDEFINED status and force copy methods that always transfer data

[wertysas]

This PR adds new features to Field API that allow the user to make data copies without Field API tracking the status of the data. The new features addresses the issue with Add support for blocked offload of fields to device #83, where the proposed changes could break the internal status tracking of fields.

Four new data transfer methods are added that always copies the fields:

• SUBROUTINE GET_HOST_DATA_FORCE(SELF, PTR, QUEUE)
• SUBROUTINE SYNC_HOST_DATA_FORCE(SELF, QUEUE)
• SUBROUTINE GET_DEVICE_DATA_FORCE(SELF, PTR, QUEUE)
• SUBROUTINE SYNC_HOST_DATA_FORCE(SELF, QUEUE)

These methods all set the field's status to UNDEFINED and the user has to set the status back to a valid status with the newly added SET_DEVICE_FRESH and SET_HOST_FRESH methods before the normal Field API routines can be used on the field.

Additionally, the optional QUEUE argument has been removed from the normal copy methods and are now only exposed to the user through the FORCE copy methods. Previously, the copy methods could be called with the QUEUE argument and the status of the field would have been updated even if the data transfer hadn't been launched. By only allowing asycnhronous copies through the FORCE methods the tracking logic of Field API will always work as expected when calling the normal copy routines.

The open draft PR, Add support for blocked offload of fields to device #83, will be rebased onto this and use the FORCE routine for offloading instead to make it impossible for a user to break the status of the fields when using non-FORCE copy methods.

[awnawab]

Thanks a lot @wertysas for this excellent piece of work 🙏 Having an expert mode that unlocks advanced functionality is a really good idea, and your implementation of exposing this via an alternate API is a very clean and simple way of achieving this.

I've left a few minor comments that I would like you to address. You've also now accrued a small conflict with a change I made to tests/async_host.F90 (sorry 😅 ), so a rebase would also be necessary.

[awnawab]

Typo: "no benefit" instead of "not benefit"

[awnawab]

typo: "pointed to" (or even better "associated to") instead of "point to"

[awnawab]

"The advanced API cedes all responsibility for data synchronisation to the user and must therefore be used with caution."

[awnawab]

Asynchronous transfers are more general than the case of overlapping communication and computation so we should update the description here.

"Using the advanced API it is possible to asynchronously transfer data, i.e. issue a non-blocking instruction. To do so..."

[awnawab]

Could you please delete this new line?

[awnawab]

SET_DEVICE_FRESH should set the device freshness bit, and unset the undefined and hostfresh bits. So I think it should be instead:

CALL SELF%SET_STATUS( IAND(SELF%GET_STATUS(), NOT(UNDEFINED)))
CALL SELF%SET_STATUS( IAND(SELF%GET_STATUS(), NOT(NHSTFRESH)))
CALL SELF%SET_STATUS( IOR( SELF%GET_STATUS() , NDEVFRESH))

And let's use more lines please to give my tired brain a better chance of understanding it 😅

[awnawab]

Same comment as SET_DEVICE_FRESH.

[awnawab]

We need a GET_HOST_DATA_OWNER_FORCE which can handle the case of delayed allocation.

[awnawab]

Hi @mlange05, @dareg, @pmarguinaud. Could you please review this PR too whenever you get the chance? Johan can then address all the comments together and rebase onto main. Thanks! 🙏

[wertysas]

Thanks a lot @wertysas for this excellent piece of work 🙏 Having an expert mode that unlocks advanced functionality is a really good idea, and your implementation of exposing this via an alternate API is a very clean and simple way of achieving this.

I've left a few minor comments that I would like you to address. You've also now accrued a small conflict with a change I made to tests/async_host.F90 (sorry 😅 ), so a rebase would also be necessary.

Thank you for the comments and suggestions for improvement @awnawab! I have addressed the comments and rebased onto main now.

[mlange05]

Looks good to me. Many thanks!

[awnawab]

Thanks a lot @wertysas for addressing the changes 🙏 This looks really good now. I just have minor comment, but other than that this looks good to go 👌 @dareg could you please give this another quick look at your earliest convenience?

[awnawab]

This probably got lost in the rebase, but shouldn't we check here if SELF%DEVPTR is associated? Even if PTR is uninitialised, DEVPTR may be initialised, so we should copy back as long as device memory is allocated.

[awnawab]

Thanks for the quick fix, G2G now 👌

[pmarguinaud]

Hello,

Could someone explain me why forcing the user to keep track of the status of field data is a progress ? And removing asynchronicity from the regular methods, is also a progress; why ?

[dareg]

Hello,
Sorry to do that kind of comment that late in the process, but we were wondering if adding that kind of complexity in field api is really necessary?

Do you have fields that are so big they don't fit on the GPU in the real code you are porting (not externalised test-case like cloudsc)?

In the benchmark we prepared for the call for tenders, we have been able to reduce the memory consumption by simply not doing all the blocks at once, and looping over a set of blocks. It can be seen here: https://github.com/pmarguinaud/IAL/blob/3522929d65a43763ad86577986abdbbb00b1a6f5/arpifs/adiab/cpg_drv.F90#L342

[awnawab]

Hi @pmarguinaud and @dareg,

We discovered there was a flaw in the original API; the status of a field is invalid between the entire time an asynchronous copy is issued and a wait is called. This gives the user a very easy way to break the field tracking mechanism. Consider the following:

CALL  FIELD%SYNC_DEVICE_RDWR(QUEUE=1)
CALL FIELD%SYNC_HOST_RDWR()

The above would result in incorrect behaviour; we may end up copying garbage back to host and subsequent calls to SYNC_HOST_RDWR would not update the data either since the status will already be NHSTFRESH. Rather than introducing additional complexity and introducing checks for proper synchronisation of asynchronous copies, we thought it is better to separate the "core" from the "advanced" API.

The core API is now limited to synchronous copies of the entire field. For the core API, FIELD_API is responsible for tracking the fields. If however the user wants to do something more sophisticated and complex, then I think it is quite fair to cede the tracking responsibility to the user and ask them to use the advanced API instead (rather than add unnecessary complexity in FIELD_API trying to cover every corner case of asynchronous offload).

Such an advanced mode would also enable us to add more features to FIELD_API, namely partial offload of fields and (eventually) streamed offloads of fields, which is necessary to overlap compute and communication.

I am aware you are already able to achieve some reduction in the device memory footprint in your benchmark. Please correct me if I am wrong, but I believe this is done through allocating smaller temporaries inside the outer block loop. Non-temporary or wrapped fields are offloaded in their entirety. We would like to add the ability to do a partial offload of any field, whether temporary or not.

Whether or not we use this functionality for our production runs is beside the point. A technical infrastructure library like FIELD_API should allow greater flexibility than simply what we need for production. Especially if such "advanced"/"optional" functionality is:

• Clearly delineated from the core functionality at the API level
• Is well explained in the Readme
• Is thoroughly tested.

I hope the above can assuage some of your concerns. Please do reach out if you have any further questions. And as always, technical feedback on the actual implementation would be greatly appreciated.

[awnawab]

Good morning @pmarguinaud and @dareg,

Have you had another chance to look at this PR? Could you please provide feedback at your earliest convenience? I would really like to merge this soon.

[dareg]

Hi,
In the example you've shown:

CALL FIELD%SYNC_DEVICE_RDWR(QUEUE=1)
CALL FIELD%SYNC_HOST_RDWR()

As you said, the status of a field is invalid while an async transfer is pending. So it seems to me that the solution is to always call the WAIT_FOR_ASYNC_QUEUE subroutines when you want to make use of a field that have been transferred asynchronously, as stated in our documentation.

CALL FIELD%SYNC_DEVICE_RDWR(QUEUE=1)
!do other stuff while the transfer is happening
CALL WAIT_FOR_ASYNC_QUEUE(1) !wait for the transfer to finish if it wasn't already
CALL FIELD%SYNC_HOST_RDWR()

Is it not enough to prevent using halfl-transferred data?

On your second point. I think we have a divergent view on what field api should be. I think we should not aim for something very flexible, general, or versatile. In the contrary we should aim for something tailored specifically for the porting of IFS/IAL on GPU, with the simplest[1] field api library we can get away with in order to ease the maintenance burden that will arise in the future.

I think we should constrain ourself to make field api work well, first and foremost, on production code.

[1] And I think it's already quite complex.

[awnawab]

Hi Judicaël,

Thanks for your reply. Yes sure, adding an explicit wait solves the problem. I personally think the API should by design not allow the user to make mistakes, but I don't have very strong feelings on it and I would be happy to restore the QUEUE argument to the standard API if that helps unblock things.

You didn't however address one of the main points in my comment so I'll say it again more clearly. Back in December we filed #68 explaining that we want to add the capability to split a copy into multiple streams and overlap communication and computation. That issue was referenced in PRs #69 and #83, so we have been talking about it for a while.

Overlapping communication and computation can potentially have significant performance benefits, so we are very keen to have this capability in FIELD_API. It has to be in the main trunk and not in a fork because evaluating the performance benefits on different kernels and architectures will be an ongoing medium-to-long-term process.

Please reevaluate the current PR in light of the above clarification. It is a key part of potentially very powerful new capability that we have been working on (and signposting to you) for months.