-
Notifications
You must be signed in to change notification settings - Fork 164
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 RV64F compilation, simplify fmv implementation, and make nan boxing functions generic #594
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nit: The title of the commit sounds like there is an implementation bug. Maybe Fix RV64F compilation and simplify fmv implementation
?
Change LGTM.
assert(xlen >= 64); | ||
let rs1_val_D = F(rs1)[63..0]; | ||
let rd_val_X : xlenbits = sign_extend(rs1_val_D); | ||
X(rd) = rd_val_X; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please keep the extends/truncates, they generalise the code to support xlen != 64 and flen != 64 (RV128 or RVQ), whereas yours assumes xlen == flen == 64.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is, the only code changes needed should be to add && flen >= 64
to the asserts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. This will need to be reverted in #445, so no reason to remove it here. Simplifying it to all happen in-line like you did with F seems cleaner though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm good point! I think it was technically missing the nan_box
, presumably because the Q version wasn't available here so I added it, and pulled the code from #448 to make the nan boxing functions support all of the float sizes.
let rd_val_X : xlenbits = sign_extend(rs1_val_S); | ||
X(rd) = rd_val_X; | ||
// A narrower n-bit transfer out of the floating-point registers will | ||
// transfer the lower n bits of the register ignoring the upper FLEN-n bits. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see the point of these comments? These behave exactly as one would expect, and are entirely consistent with the rest of the ISA.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is, reading a 32-bit value reads that 32-bit sub register, and writing one either sign-extends, in the case of integer destinations, or nan boxes, in the case of float destinations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it wasn't meant to contradict any expectations - it's a direct quote from the spec; just explain why the code was doing what it is (because it's not obvious IMO unless you are an expert). I can remove it if you like. Or maybe put it in quotes?
This simplifies the code and allows easily supporting the Q extension.
Simplify the implementations with fewer intermediate variables, and fix compilation of RV64F. I also added relevant quote from the spec because the spec for these instructions is very confusing. This is a prime candidate for getting Sail code into the spec.
60448aa
to
feb2e2d
Compare
Simplify the implementations with fewer intermediate variables, and fix compilation of RV64F.
I also added relevant quote from the spec because the spec for these instructions is very confusing. This is a prime candidate for getting Sail code into the spec.
Fixes #556