Skip to content

Track updated status for each vector in multivector. #260

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

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

pelesh
Copy link
Collaborator

@pelesh pelesh commented Apr 19, 2025

Vector class object is a multivector that allows access to individual vectors separately. It is plausible to expect that individual vectors could be updated individually and some of them may have device, while others may have host memory space updated. However, current implementation of the Vector class allows only for setting update flags for all vectors together, regardless of how they are accessed and updated. This is a bug, see #259.

This PR provides following:

Checklist:

  • All tests pass. Code tested on
    • CPU backend
    • CUDA backend
    • HIP backend
  • Code compiles cleanly with flags -Wall -Wpedantic -Wconversion -Wextra.
  • The new code follows Re::Solve style guidelines.
  • There are unit tests for the new code.
  • The new code is documented.
  • The feature branch is rebased with respect to the target branch.

@pelesh pelesh added bug Something isn't working enhancement New feature or request hip cuda labels Apr 19, 2025
@pelesh pelesh added this to the Release 0.99.2 milestone Apr 19, 2025
@pelesh pelesh self-assigned this Apr 19, 2025
@shakedregev
Copy link
Collaborator

I see not all checkmarks are there. Is this still a draft?

Copy link
Collaborator

@kswirydo kswirydo left a comment

Choose a reason for hiding this comment

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

First -- this is not a bug. What you are adding is a new feature. There is nothing wrong with current approach except ... it is limiting in some cases.

@@ -280,13 +307,32 @@ namespace ReSolve { namespace vector {
{
using namespace ReSolve::memory;

bool all_gpu_updated = gpu_updated_[0];
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 if I follow the logic here. If the length of gpu_updated and 'cpu_updated' is k, shouldnt we just check every vector separately and copy, if needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good question! This function syncs the entire multivector at once. It can do that only if all vectors in the multivector have the same updated status. Otherwise, it should exit with an error. Next few lines check if it is the case. After that, it is safe to use all_gpu_updated.

The check itself may add some overhead. Perhaps it should be enabled only in debug mode?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay, I see what you are doing.

Copy link
Collaborator

@shakedregev shakedregev left a comment

Choose a reason for hiding this comment

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

Looks good and tests pass, but maybe we want a test for this specifically? Separate PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working cuda enhancement New feature or request hip
Projects
None yet
3 participants