Corrected the swapped n and p bits in the ALU nzp computation, and the unsigned subtration overflow.#53
Open
Torrey0 wants to merge 1 commit intoadam-maj:masterfrom
Open
Corrected the swapped n and p bits in the ALU nzp computation, and the unsigned subtration overflow.#53Torrey0 wants to merge 1 commit intoadam-maj:masterfrom
Torrey0 wants to merge 1 commit intoadam-maj:masterfrom
Conversation
…ming unsigned subtration overflow.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The previous line flipped the location of the bits for n and p. The ISA states they are nzp, but the comparison: alu_out_reg <= {5'b0, (rs - rt > 0), (rs - rt == 0), (rs - rt < 0)};
checks postive, zero, negative.
The sample code for matMul, which used a BRn, worked by coincidence.
CMP R9, R2
BRn LOOP
The computation appeared to work for both cases (k<N and k=N) that it needed to:
For iterating, when k < N, for example, k = 1, N = 2:
{0, (1 - 2) > 0, (1 - 2) == 0, (1 - 2) < 0}
= {0, (255) > 0, 1 == 0, 1 < 0}
= {0, 1, 0, 0}
This comes out ok, matching the targeted nzp, but only because 1-2 is computed unsigned, resulting in 255.
As an example where the old HDL gives incorrect results:
Inverting both the comparison and branch should result in the same behavior. However, if the sample code did this, and performed:
CMP R2, R9
BRzp LOOP
The results would be different. The computation would be:
{0, (2-1) > 0, (2-1) == 0, (2-1) < 0}
= {0, 1, 0, 0}
Now, this does not match the zp flag, and matMul will exit on the very first iteration, and not be computed correctly.
The new version computes the bits in the correct order and avoids any unsigned subtraction overflow.