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

[Bug report] miss illegal instruction exception when rd of MULHU is the same as rs1 or rs2 #885

Open
chenc6 opened this issue May 25, 2022 · 11 comments
Assignees
Labels
Component:RTL For issues in the RTL (e.g. for files in the rtl directory) Status:New Newly created issue, nobody has looked at it yet. Type:Bug For bugs in the RTL, Documentation, Verification environment or Tool and Build system

Comments

@chenc6
Copy link

chenc6 commented May 25, 2022

Hi,

The RISC-V ISA Volume I, 20190608, page 43 mentions: "MULH[[S]U] rdh, rs1, rs2; MUL
rdl, rs1, rs2 (source register specifiers must be in same order and rdh cannot be the same as rs1 or rs2)"
image

Our testcase shows cva6 will not throw illegal instruction exception in this case while spike does at line 84.
image

The testcase mem file, rtl trace log, and spike trace log are attached.
missexcept.zip

Thank you

@MikeOpenHWGroup MikeOpenHWGroup added Component:RTL For issues in the RTL (e.g. for files in the rtl directory) Type:Bug For bugs in the RTL, Documentation, Verification environment or Tool and Build system Status:New Newly created issue, nobody has looked at it yet. labels Feb 11, 2023
@MikeOpenHWGroup
Copy link
Member

Hi @JeanRochCoulon , this issue is more than 7 months old and has not received any updates. It looks like an important issue to me. Please assign the right person to move this along.

@JeanRochCoulon
Copy link
Contributor

@AyoubJalali You should meet this case doing the ISACOV verification. Does ISACOV cover this case ?

@AyoubJalali
Copy link
Contributor

@JeanRochCoulon It should be, but i'll look at it

@AyoubJalali
Copy link
Contributor

@chenc6 hello, so I run a test with :
main: mulhsu zero, zero, zero
mul zero, zero, zero
slti zero, t6, -972
div ra, s2, s8
andi s0, s9, -182
or t6, a3, a5
auipc t4, 691756
mul tp, s3, t1
add t0, a6, zero
srli a7, a2, 7
sub s5, s11, s11
srai s7, s4, 26
or a2, sp, t1
remu ra, t2, a1
divu a3, sp, s1
slli s3, s4, 2
srl s4, s3, t2
rem tp, s1, a3
div ra, t5, t5
j test_done

and i didn't get any illegal instructions exception on spike side as you see here :

image

so could you providing me with the source file .S, to try to reproduce the problem if it still.
@JeanRochCoulon just to answering your question it's cover as far as I know

@chenc6
Copy link
Author

chenc6 commented Mar 14, 2023

@AyoubJalali Sorry, I couldn't find the .S file, but it should be easy to reproduce because the issue is only related to one instruction.

Maybe you can change the instruction from mulhsu to mulhu, such that the binary code will be 02003033 but not 02002033?

@AyoubJalali
Copy link
Contributor

@chenc6 I don't thing also it's a problem, because i covered all possible combinations between all multiplications instructions with all values of rx registers :

image

So could the problem be in spike version, so which version you use for this test ?

@chenc6
Copy link
Author

chenc6 commented Mar 14, 2023

@AyoubJalali the version of spike may be the problem.
I just tried the most recent version of spike: commit a35865f0f5559d8af81920b2d832e15af6caa123 and still see the illegal instruction exception.
Since spike is modified based on the ISA user manual, if the relevant description is still in the manual, spike will follow it.

@JeanRochCoulon
Copy link
Contributor

@chenc6 and @AyoubJalali The RISCV specification tells us that "rdh cannot be the same as rs1 or rs2". It is not clear to me if an illegal instruction must be triggered or not. Does the RISCV or the CVA6 ISA specification tells us what is the behaviour in that case?

@ASintzoff
Copy link
Contributor

@JeanRochCoulon @chenc6 @AyoubJalali The CVA6 behaves correctly. There is no reason to raise an illegal instruction exception on MULH[[S]U] instruction.

The specification only warns the software developer when using MULH[[S]U] and MUL instructions. If rdh is the same as rs1 or rs2 for MULH[[S]U], then the following MUL will compute a result based on a modified rs1 or rs2. Therefore, in such case, the MUL result will not be the expected one.

@AyoubJalali
Copy link
Contributor

@JeanRochCoulon I don't think the cva6 should raised an illegal instruction it's just a recommendation if you want the lower & upper part of the multiplication you should use a sequence mentioned in the RISCV spec.

@JeanRochCoulon
Copy link
Contributor

Understood. Thank for your investigation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component:RTL For issues in the RTL (e.g. for files in the rtl directory) Status:New Newly created issue, nobody has looked at it yet. Type:Bug For bugs in the RTL, Documentation, Verification environment or Tool and Build system
Projects
None yet
Development

No branches or pull requests

5 participants