Red2Band: use well-formed reflector for internal panel computations#1348
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the reduction to band algorithm to use well-formed reflectors for internal panel computations, addressing both local and distributed implementations while extracting common operations in preparation for future refactoring.
Key changes include:
- Extracted common reflector scaling operations into a reusable
scaleReflectorfunction - Modified panel computation to use well-formed reflectors (head = 1) temporarily during computations
- Simplified function signatures by removing unused parameters and improving parameter types
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| void updateTrailingPanel(const bool has_head, const std::vector<matrix::Tile<T, D>>& panel, SizeType j, | ||
| const std::vector<T>& w, const T tau, const std::size_t begin, | ||
| const std::size_t end) { | ||
| const TileElementIndex index_el_x0(j, j); | ||
|
|
||
| bool has_first_component = has_head; | ||
|
|
||
| const T* w, const T tau, const std::size_t begin, const std::size_t end) { |
There was a problem hiding this comment.
The parameter type changed from const std::vector<T>& w to const T* w. This is a breaking change that removes type safety and loses size information. Consider using std::span<const T> or keeping the vector reference to maintain type safety.
There was a problem hiding this comment.
This change is part of the refactoring needed for #1338 where, to group communications, w is just a slice of a vector (e.g. algo_data[2:2+pt_cols]).
std::span might actually be the solution, but we have to wait for c++20.
|
|
||
| // STEP3: update trailing panel (multi-threaded) | ||
| updateTrailingPanel(has_head, tiles, j, w[0], taus({j, 0}), begin, end); | ||
| updateTrailingPanel(has_head, tiles, j, w[0].data(), taus({j, 0}), begin, end); |
There was a problem hiding this comment.
Using .data() to convert from vector to raw pointer removes bounds checking and type safety. The function signature change appears to be unnecessary since the original vector reference was more type-safe.
| updateTrailingPanel(has_head, tiles, j, w[0].data(), taus({j, 0}), begin, end); | |
| updateTrailingPanel(has_head, tiles, j, w[0], taus({j, 0}), begin, end); |
|
|
||
| // STEP3: update trailing panel (multi-threaded) | ||
| updateTrailingPanel(has_head, tiles, j, w[0], taus({j, 0}), begin, end); | ||
| updateTrailingPanel(has_head, tiles, j, w[0].data(), taus({j, 0}), begin, end); |
There was a problem hiding this comment.
Using .data() to convert from vector to raw pointer removes bounds checking and type safety. The function signature change appears to be unnecessary since the original vector reference was more type-safe.
| updateTrailingPanel(has_head, tiles, j, w[0].data(), taus({j, 0}), begin, end); | |
| updateTrailingPanel(has_head, tiles, j, w[0], taus({j, 0}), begin, end); |
albestro
changed the title
|
cscs-ci run |
|
Just double checked with a quick benchmark that this is going to be neither an harm nor a benefit performance-wise. It is more for the sake of the cleanup/refactoring. 
I would say that the differences are almost null and probably due to noise. |
msimberg
left a comment
msimberg
left a comment
There was a problem hiding this comment.
Just a question about the use of optionals. The rest of the changes I can't judge too well, but looks good to me at a high level.
NB for the C++20/span question #1346 works well in DLA-Future, but still needs some verification in CP2K. I wouldn't wait for that to be able to use std::span, but at least according to https://en.cppreference.com/w/cpp/compiler_support.html#C.2B.2B20_library_features we should be able to unconditionally use std::span once that's in.
This PR