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

Address Wrong results using Integer Matmul Kernels after Matrix resizing #701

Merged
merged 1 commit into from
Jun 28, 2024

Conversation

resting-dove
Copy link
Contributor

@resting-dove resting-dove commented Apr 25, 2024

Fixes #700
#700 mentions an error in the integer MatMul kernels. This PR gives the following correct result for the mentioned example:

DenseMatrix(3x3, int64_t)
1 0 0
0 2 0
0 0 3
DenseMatrix(3x3, int64_t)
1 0 0
0 4 0
0 0 9
DenseMatrix(3x3, int64_t)
1 0 0
0 4 0
0 0 9
DenseMatrix(3x1, int64_t)
1
2
3

@resting-dove
Copy link
Contributor Author

In c932d25 I added a regression test for the wrong result in the MatMul operation. This revealed that the error also occurs under some circumstances for floating point value types. Specifically when vectors are sliced out of a matrix and then vector vector multiplied.

The later commit 6bc8d14 applies clangd formatting to the areas that I changed in MatMul.cpp. It seems that this is not the formatter that was used on this file previously. Please feel free to let me know what formatter is used, or simply go without that commit.

@corepointer
Copy link
Collaborator

Thx for detecting, reporting and fixing it right away. Good catch 👍

I'm not a fan of reviewing PRs where the formatter's gone haywire as it makes spotting actual relevant changes hard. But as we don't have any strict requirements there (yet?) and it occasionally happens to #me too, I won't complain.

I'll merge it later if the tests run without issue.

@resting-dove
Copy link
Contributor Author

Thanks for taking a look at this!
I'm sorry it didn't clear all the tests but I'm glad it caught that there's still some issues after my PR.

The rowskip is being ignored if the vectors are not views, which is wrong as far as I understand it --> Change to same behaviour whether vectors are views or not.
Also the the rhs of gemv is a vector, so it gets the skiprow logic from the dot product.

However, now some of the algorithms are breaking but I cannot determine how I have caused that or how to fix it.
I might have some more time to investigate next week but for now I apologise for having to leave this PR in such an unfinished state for now.

@corepointer
Copy link
Collaborator

Considering whether it is a view matrix or not is important because in a vectorized pipeline, input might seem like a vector but has the memory layout of a matrix. Interestingly, the tests seem to run fine now.

@resting-dove
Copy link
Contributor Author

Thanks for the clarification! I think also when vectors are just views of larger matrices the skip row should be applied in case the vector is transposed as is done after this PR.

The tests are all passing on my system as well now. I had run bin/run_tests directly before and the daphnelib tests failed since I hadn't set the global variables.

@corepointer corepointer added this to the v0.3 milestone Jun 28, 2024
corepointer pushed a commit to resting-dove/daphne that referenced this pull request Jun 28, 2024
…nels after Matrix resizing

* Use rowSkip instead of row length for Matrix memory addresses
* regression test matmul after slice
* Skiprow to be applied whether vectors are views or not

Resolves daphne-eu#700
Closes daphne-eu#701
@corepointer corepointer force-pushed the matmul_kernel_integer branch from f41a73e to 0e6d4ca Compare June 28, 2024 11:11
…nels after Matrix resizing

* Use rowSkip instead of row length for Matrix memory addresses
* regression test matmul after slice
* Skiprow to be applied whether vectors are views or not

Resolves daphne-eu#700
Closes daphne-eu#701
@corepointer corepointer force-pushed the matmul_kernel_integer branch from 0e6d4ca to 47e2621 Compare June 28, 2024 11:17
@corepointer corepointer added the bug A mistake in the code. label Jun 28, 2024
@corepointer corepointer merged commit 47e2621 into daphne-eu:main Jun 28, 2024
2 checks passed
@corepointer
Copy link
Collaborator

LGTM - thx for fixing this @resting-dove. I have squashed the commits and removed the one with the clangd formatting changes as these changes were distracting and not so ideal imho (2 space indent instead of 4, etc).
All tests ran fine locally and with GitHub Actions.

@corepointer
Copy link
Collaborator

Btw you might want to fix your git config as this and the previous commit (911c722) use different name/email settings.

Garic152 pushed a commit to Garic152/daphne that referenced this pull request Jul 9, 2024
…nels after Matrix resizing

* Use rowSkip instead of row length for Matrix memory addresses
* regression test matmul after slice
* Skiprow to be applied whether vectors are views or not

Resolves daphne-eu#700
Closes daphne-eu#701
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A mistake in the code.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong results using Integer Matmul Kernels after Matrix resizing
2 participants