Skip to content

Conversation

@rasolca
Copy link
Collaborator

@rasolca rasolca commented Sep 3, 2025

A tune parameter for the default AllocationLayout has been added too.

@rasolca rasolca self-assigned this Sep 4, 2025
@rasolca rasolca marked this pull request as ready for review September 4, 2025 11:41
@rasolca
Copy link
Collaborator Author

rasolca commented Sep 4, 2025

cscs-ci run

Copy link
Collaborator

@albestro albestro left a comment

Choose a reason for hiding this comment

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

The main comment I have is about the possible semantic confusion we might have between MatrixAllocation and AllocationType. I'm not referring to the naming of the classes, but more on the current names of the functions.

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.

Only minor nits, this looks nice overall.

@rasolca
Copy link
Collaborator Author

rasolca commented Sep 5, 2025

As it was spread on multiple conversation I answer here.

@albestro
Please note that matrix allocation specifications, allocation layout and layout are different.

The previous use of the name MatrixAllocation happened to not be a good choice.
Therefore any change in the code from MatrixAllocation -> AllocationLayout is simply due to the rename of the enum. This can create a bias when seeing the change
MatrixAllocation allocation() -> AllocationLayout allocation()
But nothing has changed beside the name of the enum.

MatrixAllocation is now used to collect all the useful specs which can modify the way the matrix buffers is allocated. However not all the options have an impact on how the matrix is used. How the leading dimension was picked and (in the future) if the matrix is allocated on demand don't have any effect on how the matrix is used.

Regarding the allocation member I decided to keep the previous name.
layout (which can work in the MatrixAllocation unit tests) is in the matrix context a even worse option. Pre-allocated matrices use a layout object for the setup. Therefore the user would expect an even more detailed object returned.
Feel free to suggest any other meaningful option.

rasolca and others added 3 commits September 5, 2025 17:54
Co-authored-by: Alberto Invernizzi <[email protected]>
Co-authored-by: Mikael Simberg <[email protected]>
@msimberg msimberg moved this from In Progress to Review in DLA-F Planning Sep 11, 2025
@github-project-automation github-project-automation bot moved this from Review to In Progress in DLA-F Planning Sep 11, 2025
@msimberg msimberg moved this from In Progress to Review in DLA-F Planning Sep 11, 2025
@rasolca
Copy link
Collaborator Author

rasolca commented Sep 12, 2025

@msimberg @albestro, I modified the MatrixAllocation constructor with the new version that is needed for adding the third parameter later. Please have a look. 3c90c00

@rasolca rasolca force-pushed the rasolca/matrix-constructors branch from bdd0637 to 8a0e533 Compare September 12, 2025 16:23
@rasolca
Copy link
Collaborator Author

rasolca commented Sep 23, 2025

@albestro @msimberg please have a quick look to e0ddcee

@rasolca
Copy link
Collaborator Author

rasolca commented Sep 23, 2025

cscs-ci run

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.

Renaming looks as discussed, thanks @rasolca!

@github-project-automation github-project-automation bot moved this from Review to In Progress in DLA-F Planning Sep 24, 2025
@rasolca rasolca requested a review from albestro October 27, 2025 16:27
Comment on lines +18 to +19
inline std::string copy_to_lower(const std::string& str) {
std::string s(str);
Copy link
Collaborator

Choose a reason for hiding this comment

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

very minor consideration

Suggested change
inline std::string copy_to_lower(const std::string& str) {
std::string s(str);
inline std::string copy_to_lower(std::string s) {

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.

3 participants