riscv64: fix remu (signed->unsigned) and remw/remuw dividend width - #83
Open
DORA-B wants to merge 1 commit into
Open
riscv64: fix remu (signed->unsigned) and remw/remuw dividend width#83DORA-B wants to merge 1 commit into
DORA-B wants to merge 1 commit into
Conversation
The RV64M remainder lifters were wrong for three of the four cases: * remu (OP, funct3=0b111) used Iop_DivModS64to64 -- the signed divmod, identical to rem -- so it returned the SIGNED remainder. Wrong whenever the signed and unsigned remainders differ (~60% of random operand pairs). VEX has no Iop_DivModU64to64, so this zero-extends rs1 to 128 bits and uses the unsigned 128/64 divmod (Iop_DivModU128to64); its high 64 bits are the unsigned remainder. * remw / remuw (OP-32, funct3=0b110 / 0b111) passed getIReg64(rs1), the full 64-bit register, as the dividend of a 64->32 divmod. These operate on rs1[31:0], so the dividend must be the sign/zero-extension of the low 32 bits, exactly as divw/divuw already do via getIReg32(rs1). Fixed to feed 32Sto64 / 32Uto64 of getIReg32(rs1). div/divu/divw/divuw and 64-bit rem were already correct. Verified by rebuilding pyvex against the patched VEX: all 8 div/rem ops match the RISC-V Unprivileged ISA (M extension) and the Sail formal model over 1500 random + edge inputs (0 mismatch); previously remu ~60% wrong and remw/remuw ~100% wrong.
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 RV64M remainder lifters were wrong for three of the four cases:
remu (OP, funct3=0b111) used Iop_DivModS64to64 -- the signed divmod, identical to rem -- so it returned the SIGNED remainder. Wrong whenever the signed and unsigned remainders differ (~60% of random operand pairs). VEX has no Iop_DivModU64to64, so this zero-extends rs1 to 128 bits and uses the unsigned 128/64 divmod (Iop_DivModU128to64); its high 64 bits are the unsigned remainder.
remw / remuw (OP-32, funct3=0b110 / 0b111) passed getIReg64(rs1), the full 64-bit register, as the dividend of a 64->32 divmod. These operate on rs1[31:0], so the dividend must be the sign/zero-extension of the low 32 bits, exactly as divw/divuw already do via getIReg32(rs1). Fixed to feed 32Sto64 / 32Uto64 of getIReg32(rs1).
div/divu/divw/divuw and 64-bit rem were already correct. Verified by rebuilding pyvex against the patched VEX: all 8 div/rem ops match the RISC-V Unprivileged ISA (M extension) and the Sail formal model over 1500 random + edge inputs (0 mismatch); previously remu ~60% wrong and remw/remuw ~100% wrong.
Reproduction (no build needed to observe the wrong IR)
The lifted IR shows the defect directly: