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

ReductionToBand, BTReductionToBand, TFactor: Taus on GPU #1304

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

albestro
Copy link
Collaborator

@albestro albestro commented Mar 5, 2025

The target of this PR is moving TAUS from being stored on CPU to be stored on GPU memory.
It's more a cleanup than an optimisation.

Changelog:

  • (trivially) some changes in the API (and test accordingly too)
  • reduction to band still computes taus on CPU, but then it copies them immediately to GPU. This required allocation of an additional support matrix on CPU (i.e. ComputePanelHelper<GPU>::mat_taus_cpu).
  • in TFactor a couple of copies have been skipped: one becuase it was a leftover, the other because now having taus on GPU they can be used in-place without copying them in the diagonal of the TFactor tile.
  • in TFactor Since CPU and GPU differs a bit in the implementation, it is now explicit, by making them private, that loop_gemv and loop_trmv are internal functions. These clarifies that these functions might have different signatures between the two different backends. Actually, in GPU version of loop_gemv, taus parameter was superfluous so it has been dropped now.

TODO

  • Run benchmarks

@albestro
Copy link
Collaborator Author

cscs-ci run

@codecov-commenter
Copy link

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.07%. Comparing base (7ce9103) to head (4f2b707).

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1304   +/-   ##
=======================================
  Coverage   95.07%   95.07%           
=======================================
  Files         141      141           
  Lines        8654     8656    +2     
  Branches     1110     1110           
=======================================
+ Hits         8228     8230    +2     
  Misses        239      239           
  Partials      187      187           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@albestro albestro marked this pull request as ready for review March 25, 2025 16:36
@albestro albestro requested review from rasolca, msimberg and RMeli March 26, 2025 09:33
Copy link
Collaborator

@msimberg msimberg left a comment

Choose a reason for hiding this comment

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

Any idea about performance changes? Should have no effect, but have you tested if that's the case?

@albestro
Copy link
Collaborator Author

Any idea about performance changes? Should have no effect, but have you tested if that's the case?

It's a good point. I still have it in my plan, but before doing that I'm double checking HMM Heterogenous Memory Management 😉 Let me add a todo/reminder in the PR 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

TFactor: Store taus on GPU for reduction to band and its back transformation
4 participants