Fix incorrect MULH/MULHU/MULHSU calculation logic in 64-bit ALU#434
Merged
mortbopet merged 3 commits intomortbopet:masterfrom Feb 18, 2026
Merged
Conversation
Owner
|
Great! Mind adding (rewriting) the program that you've used to test into the 64-bit test suite? |
The ALU implementation for MULH, MULHU, and MULHSU used a fixed 32-bit shift logic (>> 32) and casted operands to 64-bit integers. This logic is incorrect for RV64. In 64-bit mode, multiplying two 64-bit intergers produces a 128-bit result. The original codes would return the upper half of the lower 64-bits (bits 32-63) instead of the actual high 64-bits (bits 64-127), which is incorrect. This commit fixes the issue by using `__int128_t` and `unsigned __int128` for intermediate calculation when XLEN is 64, and also shifting the result by 64 bits to correctly retrieve the high part of the product. Signed-off-by: brian049 <brianshih1203@gmail.com>
Signed-off-by: brian049 <brianshih1203@gmail.com>
c2490db to
4c4ffec
Compare
Contributor
Author
|
Hello @mortbopet , I've added the test to the 64-bit test suite as requested. Additionally, I noticed the CI failed on the Windows build (due to MSVC not supporting __int128) and flagged some clang-format errors. I went ahead and included fixes for those issues as well to ensure all checks pass. |
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.
This PR fixes a bug in the ALU implementation where
mulh,mulhu, andmulhsuinstructions were calculating incorrect results when running on a 64-bit processor model.The original implementation shared the same logic for both 32-bit and 64-bit architectures (take
mulhfor example):For RV64, this logic is flawed because it truncates the 128-bit product (64-bit * 64-bit) into a 64-bit container and it shift by only 32 bits, which means returning bits [63:32] of the result instead of [127:64].
The commit updated
rv_alu.hto check forXLEN. IfXLEN=64, the ALU now uses__int128_tfor calculation and shifts the result by 64 bits to correctly retrieve the high part of the product.Verification
I verified the fix using a custom assembly program provided in issue#344, which performs multiplication that exceeds 64 bits.
Assembly Codes for Testing (from Issue #344)
Operand 1 (x8):
0x00000058000001feOperand 2 (x9):
0x000000b800000000Expected High Part (x11):
0x0000000000003f40Before Fix:
Result in x11:
0x0000000000016E90(Matches bits [63:32] of the product)After Fix:
Result in x11:
0x0000000000003F40(Matches the expected 128-bit high part)