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

Decouple RegisterMatrix from Blaze #49

Merged
merged 5 commits into from
Nov 8, 2024

Conversation

mkatliar
Copy link
Owner

@mkatliar mkatliar commented Nov 6, 2024

Fixes #15

@mkatliar mkatliar linked an issue Nov 6, 2024 that may be closed by this pull request
@mkatliar mkatliar requested a review from roversch November 6, 2024 08:07
Copy link
Collaborator

@roversch roversch left a comment

Choose a reason for hiding this comment

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

One small remark regarding nomenclature, and one question.

@@ -12,17 +12,16 @@

namespace blast :: benchmark
{
template <typename T, size_t M, size_t N, bool SO>
template <typename T, size_t M, size_t N, StorageOrder SO>
static void BM_RegisterMatrix_ger_nt(State& state)
{
using Kernel = RegisterMatrix<T, M, N, SO>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I only see this now, but technically a 'kernel' is the operation, i.e. dgemm, not the data you're operating on.

Copy link
Owner Author

Choose a reason for hiding this comment

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

True. This wording remains for histerical reasons.


#pragma once

#include <blaze/math/StorageOrder.h>


namespace blast
{
enum StorageOrder : bool
Copy link
Collaborator

Choose a reason for hiding this comment

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

bool? will there never be more StorageOrders than two?

Copy link
Owner Author

@mkatliar mkatliar Nov 8, 2024

Choose a reason for hiding this comment

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

Good point. The StorageOrder term was borrowed from Blaze, where it meant only row-major or column-major. Now I think that it could be a more complex thing, that would include panel-based and possible other storages. It could be a class that defines how to calculate memory offset given row and column indices, similar to LayoutPolicy in std::mdspan. Then we would not need special classes for panel matrices, because the StorageOrder parameter of the matrix class would handle accessing the elements. Implementing it would also solve #6 . Doing it in one step though would be a too big change, so I just decoupled StorageOrder from Blaze for now.

@mkatliar mkatliar merged commit 0aa44cc into master Nov 8, 2024
2 checks passed
@mkatliar mkatliar deleted the decouple_register_matrix_from_blaze branch November 8, 2024 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Decouple RegisterMatrix from Blaze
2 participants