Skip to content

Merge dot products in PIPECG #1908

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 8 commits into
base: develop
Choose a base branch
from
Open

Merge dot products in PIPECG #1908

wants to merge 8 commits into from

Conversation

gojakuch
Copy link
Collaborator

@gojakuch gojakuch commented Aug 6, 2025

This PR continues on the work done in PIPECG by making it fulfill its initial purpose of reducing the number of dot products in the algorithm by merging them into a singe dot operation.

@gojakuch gojakuch added the 1:ST:WIP This PR is a work in progress. Not ready for review. label Aug 6, 2025
@ginkgo-bot ginkgo-bot added reg:testing This is related to testing. mod:core This is related to the core module. mod:cuda This is related to the CUDA module. mod:reference This is related to the reference module. type:solver This is related to the solvers mod:hip This is related to the HIP module. labels Aug 6, 2025
@pratikvn pratikvn self-requested a review August 7, 2025 07:53
Copy link
Member

@pratikvn pratikvn left a comment

Choose a reason for hiding this comment

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

part 1/2. Still need to look at the algorithm itself.

Comment on lines 125 to 127
auto* w = w_unique.get();


Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
auto* w = w_unique.get();
auto* w = w_unique.get();

// GKO_SOLVER_VECTOR(r, dense_b);
// GKO_SOLVER_VECTOR(w, dense_b);
// into rw that we later slice for efficient dot product computation
auto stride = dense_b->get_stride();
Copy link
Member

Choose a reason for hiding this comment

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

nit: make it clear that this is b_stride

// m = preconditioner * w
this->get_preconditioner()->apply(w, m);
// n = A * m
this->get_system_matrix()->apply(m, n);
// TODO: merge these two dot products:
// merged dot products
// rho = dot(r, z)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// rho = dot(r, z)
// rho = dot(r, z1)

// rho = dot(r, z)
r->compute_conj_dot(z, rho, reduction_tmp);
// delta = dot(w, z)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// delta = dot(w, z)
// delta = dot(w, z2)

@gojakuch gojakuch force-pushed the feat/pipe-cg-dotmerge branch 2 times, most recently from f24265c to 23a35a3 Compare August 9, 2025 19:23
@gojakuch
Copy link
Collaborator Author

gojakuch commented Aug 9, 2025

Addressed the suggestions yesterday, rebased now.
It fails the test comparing the reference implementation to omp on my machine, which is very odd, as they seem to do the same thing. the reference implementation is now totally correct though, it passes the tests. somehow smth is wrong with step_1, still debugging it

@gojakuch gojakuch force-pushed the feat/pipe-cg-dotmerge branch from 23a35a3 to 4c7a3d5 Compare August 15, 2025 18:34
@gojakuch gojakuch requested a review from pratikvn August 15, 2025 18:36
@gojakuch gojakuch added 1:ST:ready-for-review This PR is ready for review 1:ST:run-full-test and removed 1:ST:WIP This PR is a work in progress. Not ready for review. labels Aug 18, 2025
@gojakuch gojakuch changed the title Merge the dot products in PIPECG Merge dot products in PIPECG Aug 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1:ST:ready-for-review This PR is ready for review 1:ST:run-full-test mod:core This is related to the core module. mod:cuda This is related to the CUDA module. mod:hip This is related to the HIP module. mod:reference This is related to the reference module. reg:testing This is related to testing. type:solver This is related to the solvers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants