Enable `copy` to be used with MatrixRef #1021

albestro · 2023-10-27T14:40:15Z

Basic implementation (mainly copy-paste) for a copy method involving MatrixRefs.

Currently Matrix and MatrixRef are still two independent objects, and this implies that we would have to duplicate all the code for the Matrix (more or less, e.g. MatrixRef might have an offset which creates some new corner cases) the same for MatrixRef.

This represents a problem

from the code point of view: with duplicated code, exponential number of combinations (if A,B,C are 3 matrix-like objects there are 8 combinations of Matrix vs MatrixRef calls), trivial template solution might costs a lot in terms of compile times and binaries sizes.
but also for tests: should I take almost all Matrix tests and replicate them for MatrixRef?!

Nothing new, but since it does not sound useful to spend time replicating things like that, in this PR I started with just the basic implementation of the copy and as soon as we take a decision on how to proceed, I will try to apply here too. 😉

Waiting for your ideas and suggestions!

EDIT:
After a discussion with others, we opted for using copy as a "testbed" also for the sub-matrix/pipeline mechanisms, as a "real" use-case. This means that test for copy will be more "extensive" wrt other algorithms (e.g. gemm) and we will have:

tests for Matrix: just full matrix
tests for MatrixRef and Pipeline (i.e. Matrix)
- sub-matrix
- full-matrix

For the rest of the algorithms (so gemm included), we will just test the with MatrixRef both full and sub-matrices cases (where applicable).

msimberg · 2023-10-27T15:28:33Z

Just for the record, I will benchmark these changes: master...msimberg:DLA-Future:matrix-ref-algorithm-input-shortcut-trivial-ref. Hopefully by early next week we'll know more about if we can possibly only instantiate algorithms with MatrixRef and then you could update this PR to do the same.

test/unit/matrix/test_copy.cpp

msimberg · 2023-11-13T15:06:41Z

include/dlaf/matrix/copy.h

+  namespace ex = pika::execution::experimental;
+
+  if constexpr (Source == Destination) {
+    if (&src == &dst)


I don't know if this needs to be resolved in this PR, but since this compares MatrixRef addresses and not Matrix addresses this could in the worst case lead to deadlocks if the MatrixRefs refer to the same Matrix (read and readwrite into the same task). I think one step in the right direction would be to have operator== on MatrixRef (which compares the addresses of the contained Matrix and sub distributions), but I'm not sure if it covers all possible issues.

I'd be fine with dealing with this outside of this PR.

That's a good point. I did a no-brainer copy-paste, but actually it hides a dangerous thing. How should we address this? Immediately or just open an issue? @rasolca @msimberg

I think that Submatrix pipeline should be considered as well, so it is better to work on it separately.
However it should be taken care as the tridiagonal solver might be impacted by it.
(device <-> host mirror copies in the case of multicore are skipped as they are the same matrix, however after the cleanup they will be two equal ref but different objects)

Thanks @msimberg for opening the issue!

I'll not resolve this comment hoping it will be easier in the future to spot it in case of need of reference.

rasolca

Please start to use snake_case for functions.

test/unit/matrix/test_copy.cpp

albestro · 2023-11-16T13:28:12Z

cscs-ci run

codecov-commenter · 2023-11-16T13:57:09Z

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (3097ab4) 94.10% compared to head (b1a41bf) 94.09%.
Report is 10 commits behind head on master.

Files	Patch %	Lines
include/dlaf/matrix/copy.h	81.81%	1 Missing and 1 partial ⚠️

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

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1021      +/-   ##
==========================================
- Coverage   94.10%   94.09%   -0.02%     
==========================================
  Files         146      146              
  Lines        8843     8856      +13     
  Branches     1121     1125       +4     
==========================================
+ Hits         8322     8333      +11     
- Misses        326      327       +1     
- Partials      195      196       +1

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

albestro · 2023-11-16T15:16:12Z

test/unit/matrix/test_copy.cpp

+template <class ElementGetter>
+auto subValues(ElementGetter&& fullValues, const GlobalElementIndex& offset) {
+  return [fullValues, offset = sizeFromOrigin(offset)](const GlobalElementIndex& ij) {
+    return fullValues(ij + offset);
+  };
+}
+
+template <class OutsideElementGetter, class InsideElementGetter>
+auto mixValues(const GlobalElementIndex& offset, const GlobalElementSize& sub_size,
+               InsideElementGetter&& insideValues, OutsideElementGetter&& outsideValues) {
+  return [outsideValues, insideValues, offset, sub_size](const GlobalElementIndex& ij) {
+    if (ij.isInSub(offset, sub_size))
+      return insideValues(ij - common::sizeFromOrigin(offset));
+    else
+      return outsideValues(ij);
+  };
+}


These helpers might be useful elsewhere, and I will probably move them in some "common" place so that they can be used in other tests too. (Probably this will happen in #969)

…copy

In particular SubMatrix test for MC has been added (GPU yet to be adpated)

albestro · 2023-11-16T16:12:38Z

cscs-ci run

rebased on new master (for getting matrix base snake_case) and addressed snake_case in implementation too.

after this it is ready to be merged 👍🏻

albestro self-assigned this Oct 27, 2023

albestro added this to the Optimizations milestone Oct 27, 2023

albestro requested review from msimberg, rasolca, aurianer and RMeli October 27, 2023 14:41

albestro added the Type:New Feature New feature or request label Oct 27, 2023

albestro force-pushed the alby/matrix-ref-copy-local branch from 6e866b2 to 0fff1a9 Compare November 6, 2023 16:56

albestro commented Nov 6, 2023

View reviewed changes

test/unit/matrix/test_copy.cpp Outdated Show resolved Hide resolved

albestro marked this pull request as ready for review November 10, 2023 16:09

albestro force-pushed the alby/matrix-ref-copy-local branch from 53b00a7 to 7d46120 Compare November 13, 2023 08:47

msimberg approved these changes Nov 13, 2023

View reviewed changes

msimberg mentioned this pull request Nov 15, 2023

Self-copy check for MatrixRef needs to be more thorough #1035

Open

rasolca reviewed Nov 15, 2023

View reviewed changes

test/unit/matrix/test_copy.cpp Outdated Show resolved Hide resolved

test/unit/matrix/test_copy.cpp Outdated Show resolved Hide resolved

test/unit/matrix/test_copy.cpp Outdated Show resolved Hide resolved

albestro requested a review from rasolca November 16, 2023 14:18

rasolca approved these changes Nov 16, 2023

View reviewed changes

albestro commented Nov 16, 2023

View reviewed changes

albestro added 10 commits November 16, 2023 17:05

basic implementation

736f9d4

remove copy tests from matrix test

a52c10a

fix implementation: make it distributed-ready + fix skip same-matrix …

ed4a90f

…copy

WIP: starting point for test_copy

ecbd6c8

In particular SubMatrix test for MC has been added (GPU yet to be adpated)

introduce rank alignment for sub-matrix test cases

08fdd4d

start adapting also submatrix gpu test

7923f2e

add matrix-ref sub-matrix test also for gpu

83c515b

add local tests

df72939

remove template keyword as suggested

457e9b8

use matrix c'tor + use snake_case for distribution + remove typedefs

d3f9993

albestro added 3 commits November 16, 2023 17:05

factor out helper for isInSub

1b04edf

some more snake_case

ee9dc76

some more snake_case in impl

651f2f6

albestro force-pushed the alby/matrix-ref-copy-local branch from b1a41bf to 651f2f6 Compare November 16, 2023 16:11

rasolca merged commit 1b9b469 into master Nov 16, 2023

rasolca deleted the alby/matrix-ref-copy-local branch November 16, 2023 17:30

github-actions bot pushed a commit that referenced this pull request Nov 16, 2023

Doc Enable copy to be used with MatrixRef (#1021)

19a673d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable `copy` to be used with MatrixRef #1021

Enable `copy` to be used with MatrixRef #1021

albestro commented Oct 27, 2023 •

edited

Loading

msimberg commented Oct 27, 2023

msimberg Nov 13, 2023

albestro Nov 13, 2023

rasolca Nov 14, 2023

msimberg Nov 15, 2023

albestro Nov 16, 2023

rasolca left a comment

albestro commented Nov 16, 2023

codecov-commenter commented Nov 16, 2023

albestro Nov 16, 2023

albestro commented Nov 16, 2023

Enable copy to be used with MatrixRef #1021

Enable copy to be used with MatrixRef #1021

Conversation

albestro commented Oct 27, 2023 • edited Loading

msimberg commented Oct 27, 2023

msimberg Nov 13, 2023

Choose a reason for hiding this comment

albestro Nov 13, 2023

Choose a reason for hiding this comment

rasolca Nov 14, 2023

Choose a reason for hiding this comment

msimberg Nov 15, 2023

Choose a reason for hiding this comment

albestro Nov 16, 2023

Choose a reason for hiding this comment

rasolca left a comment

Choose a reason for hiding this comment

albestro commented Nov 16, 2023

codecov-commenter commented Nov 16, 2023

Codecov Report

albestro Nov 16, 2023

Choose a reason for hiding this comment

albestro commented Nov 16, 2023

Enable `copy` to be used with MatrixRef #1021

Enable `copy` to be used with MatrixRef #1021

albestro commented Oct 27, 2023 •

edited

Loading