-
Notifications
You must be signed in to change notification settings - Fork 204
Add support for Zvksh extension #862
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
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.
This is so much nicer than the existing spec! Just minor suggestions.
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.
One small nit, but looks great! A huge improvement from the version in the spec right now!
model/riscv_insts_zvksh.sail
Outdated
foreach (j from 16 to 23) | ||
w[j] = zvk_sh_w(w[j - 16], w[j - 9], w[j - 3], w[j - 13], w[j - 6]); | ||
|
||
write_velem_oct_vec(vd, SEW, vrev8([w[23], w[22], w[21], w[20], w[19], w[18], w[17], w[16]]), i); |
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.
Might be clearer to break the vrev8
out into its own line before this. Kind of easy for it to get lost otherwise when reading through this function.
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.
In regard to this discussion. Would it make sense to do something like:
write_velem_oct_vec(vd, SEW, vrev8([w[23], w[22], w[21], w[20], w[19], w[18], w[17], w[16]]), i); | |
write_velem_oct_vec(vd, SEW, | |
vrev8([w[23], w[22], w[21], w[20], w[19], w[18], w[17], w[16]]), i); |
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.
It's definitely better. Does it really not work writing it to an intermediate variable?
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.
Unfortunately not
what works is:
let 'm = 4;
let w_out : vector(8, bits('m * 8)) = vrev8([w[23], w[22], w[21], w[20], w[19], w[18], w[17], w[16]]);
write_velem_oct_vec(vd, SEW, w_out, i);
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.
Very nice code! Did any of it come from #248 (I'll add a co-authored-by if so)?
@Timmmm yes some parts are from #248. I added @charmitro as a co-author in the first commit. Usually, I make a few commits during development, and then before merging, I squash everything into the first commit, with him listed as a co-author. |
ef87c0e
to
c05d468
Compare
Co-authored-by: Charalampos Mitrodimas <[email protected]>
@pmundkur I just noticed that this PR has two approvals, so we could theoretically merge it unless there is a reason not to. The Rocq build is currently failing, I am not sure whether this needs to be resolved before merging or if it is acceptable for now. Let me know what you think. |
The Rocq output is failing because it isn't as clever as Sail about solving |
Add following instructions:
Tested with riscv-vector-tests.