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

Consistent gemm tests #16

Merged
merged 3 commits into from
Aug 12, 2024
Merged

Consistent gemm tests #16

merged 3 commits into from
Aug 12, 2024

Conversation

mkatliar
Copy link
Owner

@mkatliar mkatliar commented Aug 4, 2024

Made gemm tests consistent:

  • D = alpha * op1(A) * op2(B) + beta * C is used or implementations that support out-of-place gemm
  • C = alpha * op1(A) * op2(B) + beta * C is used or implementations that support in-place gemm only
  • op1 and op2 are chosen as "transpose" or "don't transpose" depending on what we think is fastest for each implementation
  • alpha = 1. and beta = 1. for libxsmm, because otherwise it either crashes or returns in 0 time.

Base automatically changed from xsimd to master August 5, 2024 12:18
@mkatliar mkatliar changed the title Panel gemm Consistent gemm tests Aug 5, 2024
@mkatliar mkatliar self-assigned this Aug 5, 2024
@mkatliar mkatliar requested a review from roversch August 5, 2024 12:22
@mkatliar mkatliar marked this pull request as ready for review August 5, 2024 12:22
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.

Looks fine, please answer those 2 questions

include/blast/math/panel/Gemm.hpp Show resolved Hide resolved
bench/blast/math/dense/StaticGemm.cpp Show resolved Hide resolved
@mkatliar mkatliar force-pushed the panel_gemm branch 2 times, most recently from e7c50aa to d53768d Compare August 6, 2024 11:23
@mkatliar
Copy link
Owner Author

mkatliar commented Aug 6, 2024

@roversch please re-check that gemm benchmarks are consistent between implementations, as per PR description.

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.

Just a small refactoring for libxsmm would be nice. Otherwise looks good!

Comment on lines +55 to +60
randomize(a);
randomize(b);
randomize(c);

// libxsmm behaves upredictably with alpha != 1. || beta != 1.
Real const alpha = 1., beta = 1.;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Quite some duplication. I would refactor into its own setup function or so.

Copy link
Owner Author

@mkatliar mkatliar Aug 12, 2024

Choose a reason for hiding this comment

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

Yes, there is a lot of duplicated code in benchmarks, not only here. I have created the issue #25 to address this problem later.

include/blast/math/panel/Gemm.hpp Show resolved Hide resolved
@mkatliar mkatliar merged commit 55c4737 into master Aug 12, 2024
1 check passed
@mkatliar mkatliar deleted the panel_gemm branch August 12, 2024 12:52
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.

2 participants