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

Add finer control field methods to read and write halos from accelerated devices #56

Merged
merged 14 commits into from
Mar 24, 2021

Conversation

sergisiso
Copy link
Collaborator

@sergisiso sergisiso commented Jan 18, 2021

Closes #53 by adding parameters to the read_from/write_to_device methods. The halos in the generic exchange are now passed using this finer-grain data selection.

Also:

  • Closes field%set_data() should update accelerator devices #29 : Adds write_to_device methods and fixes set_data().
  • Added a new test to check the device acceleration functionality (it is not exhaustive yet but at least it starts providing some examples and guarantees)
  • Switched to GitHub Actions
  • Switched device pointers to type(c_ptr) instead of integer(c_intptr_t). The reason is that the later is exclusive to how OpenCL works (and causes some compiler issues). The c_ptr is more generic and fits better with SYCL and Kokkos.

@sergisiso sergisiso self-assigned this Jan 18, 2021
@sergisiso sergisiso force-pushed the 53_finer_control_copies branch 4 times, most recently from e0ae84b to 5ff3bd1 Compare February 5, 2021 09:47
@sergisiso sergisiso force-pushed the 53_finer_control_copies branch from 5ff3bd1 to d3b073d Compare February 5, 2021 09:58
@sergisiso
Copy link
Collaborator Author

@arporter This is ready for review. It prepares dl_esm_inf to expose more information about the device reads and writes in order to make the MPI+device computation more efficient (and correct!).

I have already use it in manual PSycloneBench and PSyclone to prove that it provides correct results. I will clean up and submit those PR next.

Copy link
Member

@arporter arporter left a comment

Choose a reason for hiding this comment

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

Nice work Sergi, I particularly like the new test. It works out of the box for me.
Just a few very minor things to tidy up.

finite_difference/src/field_mod.f90 Outdated Show resolved Hide resolved
finite_difference/src/field_mod.f90 Outdated Show resolved Hide resolved
finite_difference/src/field_mod.f90 Show resolved Hide resolved
finite_difference/src/field_mod.f90 Outdated Show resolved Hide resolved
module virtual_device
use field_mod
use kind_params_mod, only: go_wp
use iso_c_binding
Copy link
Member

Choose a reason for hiding this comment

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

Please could you add an ONLY clause to this import to make it easier to follow the code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed

Copy link
Member

Choose a reason for hiding this comment

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

Not quite :-) I meant changing this to something like, use iso_c_binding, only: c_loc, etc. etc.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ahh, I think I must have replied thinking it was the previous comment, I did not address this suggestion the first time.

Now its done

@sergisiso
Copy link
Collaborator Author

@arporter I made the suggested changes. This is ready for another look.

Copy link
Member

@arporter arporter left a comment

Choose a reason for hiding this comment

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

Almost! Just the one outstanding comment to deal with.

module virtual_device
use field_mod
use kind_params_mod, only: go_wp
use iso_c_binding
Copy link
Member

Choose a reason for hiding this comment

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

Not quite :-) I meant changing this to something like, use iso_c_binding, only: c_loc, etc. etc.

@sergisiso
Copy link
Collaborator Author

@arporter Ready again, I missed the comment the last time.

Copy link
Member

@arporter arporter left a comment

Choose a reason for hiding this comment

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

All requested changes have been made. New test still compiles and runs OK.

@arporter arporter merged commit 848d210 into master Mar 24, 2021
@arporter arporter deleted the 53_finer_control_copies branch March 24, 2021 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants