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

Fix offset bug in matrix multiply cascade #1770

Merged
merged 4 commits into from
Sep 16, 2024
Merged

Conversation

hunhoffe
Copy link
Collaborator

@hunhoffe hunhoffe commented Sep 12, 2024

I had introduced a bug in both matrix multiply whole_array and matrix multiply cascade. In a previous PR, I fixed it for whole_array but did not realize the bug affected cascade as well until it occurred to me to double check recently.

@hunhoffe
Copy link
Collaborator Author

hunhoffe commented Sep 12, 2024

I believe the change I committed fixes the bug, but am running the whole matrix multiply sweep.sh on cascade locally to be more sure before I set this PR as ready for review. This may be related to #1751 but I did not verify (although I do know the bug can present with that type of error). This bug would not have affected matrix_vector, as matrix_vector does not contain any object_fifo_links with offsets, so at most it partially addresses the issue.

At the least, the following case failed before this commit but succeeds after this commit:

M?=640
K?=896
N?=768
m?=16
k?=32
n?=48

@hunhoffe
Copy link
Collaborator Author

hunhoffe commented Sep 13, 2024

I tried config:

M?=32
K?=512
N?=512
m?=32
k?=64
n?=32

and it passed. This may address (one of) the problems raised in the (possibly related) issue (namely, M = 32, n = 32).

@hunhoffe hunhoffe marked this pull request as ready for review September 13, 2024 17:12
@hunhoffe hunhoffe added this pull request to the merge queue Sep 16, 2024
Merged via the queue into main with commit 932a5df Sep 16, 2024
67 checks passed
@hunhoffe hunhoffe deleted the matmul-cascade-link-offsets branch September 16, 2024 17:22
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