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

Adapting to HPX V1.5 #643

Open
wants to merge 9 commits into
base: 1
Choose a base branch
from
Open

Adapting to HPX V1.5 #643

wants to merge 9 commits into from

Conversation

hkaiser
Copy link
Collaborator

@hkaiser hkaiser commented Aug 2, 2020

This patch adds some conditional changes that a required to make flecsi/1 compile with HPX V1.5

@hkaiser hkaiser marked this pull request as draft September 13, 2020 00:31
@hkaiser hkaiser changed the title Adapting to HPX V1.5 [WIP] Adapting to HPX V1.5 Sep 13, 2020
@hkaiser hkaiser changed the title [WIP] Adapting to HPX V1.5 Adapting to HPX V1.5 Sep 28, 2020
@hkaiser hkaiser marked this pull request as ready for review September 28, 2020 19:06
Copy link
Contributor

@opensdh opensdh left a comment

Choose a reason for hiding this comment

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

I haven't gone through all(!) the changes yet, but here are some initial/procedural points.

flecsi/coloring/dcrs_utils.h Outdated Show resolved Hide resolved
flecsi/execution/test/finite_difference_dense.cc Outdated Show resolved Hide resolved
flecsi/data/data_client_handle.h Outdated Show resolved Hide resolved
flecsi/utils/mpi_type_traits.h Outdated Show resolved Hide resolved
@hkaiser
Copy link
Collaborator Author

hkaiser commented Oct 9, 2020

This has been completely redone (please note I force-pushed to the underlying branch). The changes unrelated to HPX have now been separated and are available as #645. This PR has now only changes that are required to have HPX V1.5 as a backend.

@hkaiser
Copy link
Collaborator Author

hkaiser commented Oct 9, 2020

Please also note that this PR depends on laristra/cinch#187

- enable all tests for HPX
@hkaiser
Copy link
Collaborator Author

hkaiser commented Oct 17, 2020

@opensdh, @ipdemes: I have addressed all review comments as discussed. Please note that I have pushed additional changes to laristra/cinch#187.

- flyby: make FLECSI_USE_AGGCOMM=On default for HPX backend
- adding future to data_client_handle, integrate with task dependency logic
- flyby: optimize execution_policy::execute_task

auto & h = a.handle;
// Skip Read Only handles
if constexpr((SHARED_PERMISSIONS == ro) || (GHOST_PERMISSIONS == rw) ||
Copy link
Contributor

Choose a reason for hiding this comment

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

Do the EXCLUSIVE_PERMISSIONS not need to be used to setup task dependencies?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Excellent question. I essentially copied the conditions from the MPI backend, so I might have missed things. Is there an explanation of how (and which) permissions are to be interpreted to deduce dependencies? Or in general, what is encoded by the different types of permissions?

Copy link
Contributor

Choose a reason for hiding this comment

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

The MPI backend does not do dependency analysis. The check on permission is only for ghost copy purpose. Thus the task_[epi|pro]logue would skip handles that does not write to ghost or shared region.

For a more completed dependency analysis, the permission on Exclusive should be taken into account.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for this explanation. I'm still unsure about the role of the various permissions, though. Would you be able to recommend some documentation or similar to read up about this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Minor correction. GHOST_PERMISSION == rw is used to signify the values of ghost cells are computed locally (flecsale does that) and thus ghost exchange is not necessary. Again, I don't know what the implication to dependency analysis is.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ollielo
Copy link
Contributor

ollielo commented Dec 1, 2020 via email

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.

4 participants