Merging vector extension code from vector-dev to the main branch

[XinlaiWan]

Here is the pull request for merging the vector extension code to the main branch. Please comment if there are issues in the vector code, and I'll fix them soon.

[jrtc27]

I would like to review this. I am out of the country this week, and it will take a while to comb through all of this. Please do not merge this until I have reviewed it.

[vmoya-smd]

Another thing that perhaps should be documented somewhere in the repository:

• vstart support for non-memory vector operations: vstart != 0 supported or triggers illegal instruction
• mask and tail agnostic support: write 1s or identical to undistubed (current and spike implementation)
• configurability for the above and other parts of the specification, document what has been made non-configurable but is optional in the spec
• TODOs for the vector extension

[allenjbaum]

inline comments

***@***.**** commented on this pull request. Another thing that perhaps should be documented somewhere in the repository: - vstart support for non-memory vector operations: vstart != 0 supported or triggers illegal instruction - mask and tail agnostic support: write 1s or identical to undistubed (current and spike implementation) Are you saying that something is missing in the pull, is not documented or

both? e.g. Sail should be configurable as to which of those (write 1 or undisturbed) it implements when agnostic is selected, and I dimly recall that vstart support is implementation dependent. (I am unclear, from a spec point of view, whether these can vary from op to op, or even sometimes do one and sometimes another)

- configurability for the above and other parts of the specification, document what has been made non-configurable but is optional in the spec Yes, please - from an ACT standpoint, we need to be able to configure the

Sail model to do what is allowed by the spec

- TODOs for the vector extension Which should include what is non-configurable, and should be configurable.

We should also have a list of all the possible configurabillity because riscv-config YAML will need to add it to the schema.

…

------------------------------ Message ID: ***@***.***>

[XinlaiWan]

The issues I'm going to fix:

1. vstart setting and vd writing in vector load/stores when there are faults
2. reduction instructions when vl=0
3. vtype.vsew mapping
4. a vector slicing syntax issue
5. some notes about TODO configurations

[billmcspadden-riscv]

See my comments in the code review comments.

[github-actions]

Unit Test Results

712 tests  ±0   712 ✔️ ±0   0s ⏱️ ±0s
    6 suites ±0       0 💤 ±0 
    1 files   ±0       0 ❌ ±0 

Results for commit 118ad66. ± Comparison against base commit c04cf29.

♻️ This comment has been updated with latest results.

[XinlaiWan]

The issues I'm going to fix now:

1. Update some floating-point functions
2. EXTS, EXTZ renaming (this may need merging new commits from the main branch into the vector-dev branch first)

[jrtc27]

No merges for the branch being merged to master. Rebase. Otherwise history will be a mess.

[jrtc27]

git diff origin/master...origin/vector-dev | awk '/^\+\+\+/{f=$2; gsub(/^b\//, "", f); next}; /^@@ /{l=$3; gsub(/^\+/, "", l); gsub(/,.*/, "", l);} /^\+.*[ \t]$/{print f":"l":"$0} /^[ +]/{l=l+1} says there are 103 instances of trailing whitespace added, two of which aren't in V-specific files (which I've commented on explicitly). Once you've rebased to pick up the pre-commit checks then it should also flag them all for you.

There's also a huge amount of boilerplate being copied between functions here. I don't know if there's scope to reuse more of it rather than copy/paste it 10s of times. At least the loads and stores look easy enough to merge, from what I can tell. The duplication means that fixing some of my comments may be painful, but that's what happens when they get ignored on prior PRs and the code pattern I'm commenting on get copied many times over.

This is not as thorough a review as I'd like to be able to give, but that's what happens for 10k line PRs done in one go rather than broken up... I've not checked for unused functions or variables, for example, which I would do if it were tractable.

The commit messages are also a mess thanks to blindly using GitHub's squash and merge functionality, which creates extremely ugly commit messages.

[XinlaiWan]

Thanks for all the reviewers' effort! I'll fix the issues later. One thing I'd like to mention is that for the similar code patterns, we've really tried our best to reduce the duplicate code, trying to make sure all the code blocks are necessary. Vector instructions do need a lot of preparation actions before starting calculations. They need to:

1. Read SEW and LMUL from vtype
2. Check illegal instruction because almost all of the check conditions require the knowledge of SEW and LMUL setting (even register overlap checking needs LMUL to calculate register group boundaries)
3. Read source register groups to be Sail vectors. This step looks very similar for different instructions but actually different, because all the register groups have their own EEW and EMUL. In most cases they are the same as SEW and LMUL, but there are a lot of cases in which they are different and need additional calculations.
4. Read destination register group to be Sail vector because some elements may be masked and keep their original values.
5. Determine all the masked elements, inlcuding elements masked by v0, prestart elements, and tail elements.

All the above are unique actions in vector instructions, which add a lot of complexity in the code compared with scalar instructions. They contain many lines of code, and do look similar in every execute function, but they are not the same. We have more than 10 illegal instruction checking functions, which is a proof for these complex cases. Even for C models like Spike, the solution is to define a lot of code snippets as macros and replace them in many places. I totally understand that similar code patterns do not look nice for our reviewers, but they are what we have to write.

For the unit-stride and strided load/stores, unit-stride one is a special case of strided ones. The reason they're not merged together is that the vector spec defines load/stores as three categories: unit-stride, strided, and indexed. Because they're divided into different categoies, I think implementing them separately is clearer for understanding the vector spec. Also, I know that non-segment load/stores are special cases of segment load/stores, but they are still defined seperately. I think I have the same reason to implement them separately, instead of merging them into the code blocks of segment ones. Furthermore, almost all vector load/store instructions are actually special cases of the vector indexed segment load/stores, but I don't think it's necessary to merge them all together.

[XinlaiWan]

In the new commits, floating-point functions are updated in Sail and the softfloat source code has been recovered to the original version. Some coding issues are fixed according to the suggested changes. For the load/store instructions, the code of non-segment instructions is merged into segment ones as their encoding is naturally included in the segment ones.

[XinlaiWan]

I have used git rebase to sync with the main branch, and now there's no conflict between the two branches.

[jrtc27]

All these fixup commits need to be folded into the commits they're fixing, otherwise history will be a mess. Every commit in PRs should be self-standing and bug-free (beyond missing features that will be added by subsequent commits).

[XinlaiWan]

All these fixup commits need to be folded into the commits they're fixing, otherwise history will be a mess. Every commit in PRs should be self-standing and bug-free (beyond missing features that will be added by subsequent commits).

I'm not familiar with the expected rule of history management. I think I need some help from our maintainers to fold up these commits in an acceptable way after I fix all the issues.

[jrtc27]

All these fixup commits need to be folded into the commits they're fixing, otherwise history will be a mess. Every commit in PRs should be self-standing and bug-free (beyond missing features that will be added by subsequent commits).

I'm not familiar with the expected rule of history management. I think I need some help from our maintainers to fold up these commits in an acceptable way after I fix all the issues.

Search engines exist, and the Internet has countless articles on this, such as https://thoughtbot.com/blog/git-interactive-rebase-squash-amend-rewriting-history to pick the first Google result that shows for me and looks vaguely promising.

[XinlaiWan]

I have a new question about Sail syntax:
After Sail upgrading to 0.16 version, the previous way of declaring an empty vector is no longer supported. It will result in an error like this:

Type error:
model/riscv_vext_regs.sail:270.43-52:
270 |  var result : vector('n, dec, bits('m)) = undefined;
    |                                           ^-------^
    | Type vector('n, dec, bits('m)) is empty

I'd like to make sure what's the expected new way of writing this variable declaration now.

[Alasdair]

You need a constraint that 'n >= 0, otherwise you could call the function and create an undefined vector of negative length, which should be uninhabited (i.e. an empty type with no values).

[XinlaiWan]

You need a constraint that 'n >= 0, otherwise you could call the function and create an undefined vector of negative length, which should be uninhabited (i.e. an empty type with no values).

The constraints are added now. Thanks for the help!

[XinlaiWan]

I notice when reading sstatus register, it will call lower_mstatus() to get the values, so I add the call of update_VS() in it to copy the mstatus.VS into sstatus.VS. And when writing sstatus register, it will call legalize_sstatus() -> legalize_mstatus() -> lift_sstatus(), so I add the call of update_VS() in lift_sstatus() to make mstatus.VS sync with sstatus.VS. I imitated the behavior of mstatus.FS and sstatus.FS in these functions and I think the copying is done in this way.

…

On Wed, Sep 20, 2023 at 12:52 AM Martin Berger ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ On model/riscv_sys_regs.sail <#286 (comment)>: @XinlaiWan <https://github.com/XinlaiWan> Another question: why are you not copying VS into the sstatus register? The spec requires this: [image: Screenshot 2023-09-19 at 17 52 22] <https://user-images.githubusercontent.com/404397/269035171-f419f403-63b5-4746-b6f0-6c304c581bfa.png> — Reply to this email directly, view it on GitHub <#286 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AKFKAKM7B6366T67QVWAWKDX3HEV5ANCNFSM6AAAAAA2OHOROE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[martinberger]

I notice when reading sstatus register, it will call lower_mstatus() to get the values, so I add the call of update_VS() in it to copy the mstatus.VS into sstatus.VS. And when writing sstatus register, it will call legalize_sstatus() -> legalize_mstatus() -> lift_sstatus(), so I add the call of update_VS() in lift_sstatus() to make mstatus.VS sync with sstatus.VS. I imitated the behavior of mstatus.FS and sstatus.FS in these functions and I think the copying is done in this way.

Yes. What you did looks reasonable, because, as you say, the Vector spec says "It is defined analogously to the floating-point context status field, FS". (The spec could be more clear here)

[jrtc27]

I have not reviewed the code again to verify everything's been addressed, but the current commit messages are an absolute mess. They will need cleaning up before this can be merged.

[billmcspadden-riscv]

The comment, "the current commit messages are an absolute mess", is not helpful. Please request specific improvements to the commit message. WIth regards to your review: please provide your review posthaste. We have been working on this code submission for nearly a year via incremental PRs in the vector-dev branch. It's time to complete this effort. Bill Mc.

…

On Tue, Oct 10, 2023 at 4:38 PM Jessica Clarke ***@***.***> wrote: I have not reviewed the code again to verify everything's been addressed, but the current commit messages are an absolute mess. They will need cleaning up before this can be merged. — Reply to this email directly, view it on GitHub <#286 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AXROLOFTGOQZM4FGYUE2XG3X6W56LAVCNFSM6AAAAAA2OHOROGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONJWGMYDKOJSGU> . You are receiving this because you commented.Message ID: ***@***.***>

-- Bill McSpadden Formal Verification Engineer RISC-V International mobile: 503-807-9309

[jrtc27]

This is the current commit message:

These updates introduce support for the RISC-V Vector Extension.

It is a combination of 16 commits.

V extension general framework and configuration setting instructions (#191)

• V extension general framework and configuration setting instructions

• Update model/riscv_insts_vext_utils.sail

fix a typo

Co-authored-by: Nicolas Brunie [email protected]
Signed-off-by: BrighterW [email protected]

• Update model/riscv_insts_vext_vset.sail

• Revisions after Nov 22 meeting

• Update effect matching for functions in riscv_vlen.sail

• Fix code formatting issues

• Update model/riscv_insts_vext_utils.sail

Co-authored-by: Jessica Clarke [email protected]
Signed-off-by: Xinlai Wan [email protected]

• Fix coding style issues

• Update vset instructions

Signed-off-by: BrighterW [email protected]
Signed-off-by: Xinlai Wan [email protected]
Co-authored-by: Nicolas Brunie [email protected]
Co-authored-by: Jessica Clarke [email protected]

Vector load / store instructions (#198)

• Add vector load / store instructions

• Modify the implementation of SEW, LMUL, VLEN and avoid real numbers in the code

• Update vstart setting in vector load / store instructions

• Remove unnecessary assert statements in vector instructions

• Fix bugs in vleff instructions and revise coding styles

• Add guards for vector encdec clauses, Avoid redundant memory access after vector load/store failure

Vector integer/fixed-point arithmetic & mask instructions (#227)

• Add vector arithmetic & mask instructions

• Update vector EEW and EMUL checking function

• Add vector instruction illegal check functions

• Adjust code formatting for vector instruction illegal check functions

Merge approved by team at tech-golden-model meeting on 2023-03-14.

Vector floating-point instructions (#232)

• Add vector floating-point instructions

• Update vector floating-point conversion instructions

• Update copyright headers for vector extension code

---

Co-authored-by: xwan [email protected]

Vector reduction and mask instructions (#259)

• Add vector mask and reduction instructions

• Fix register overlap check in vector mask instructions

---

Co-authored-by: xwan [email protected]

update vector CSR vtype.vill setting and judgement

Summarize patterns of vector illegal instruction check

Fix issues in vector load/store and reduction operations

Clean up the V extension code and vector floating-point functions

Clean up the softfloat makefiles

Rename EXTZ and EXTS in the V extension code

Fix an issue in the V extension code for clang-format check

Fix NaN boxing issue and add notes for RVV configuration TODOs

Add default VLEN value and set vlenb CSR

Add constraints for vector variable initialization

Add mstatus.VS setting code for vector extension

I don't know how else to put it. It's 56 non-empty lines, most of which is random fixup commit messages and Co-authored-by's. There is no constructive way to put it beyond "write a proper commit message".

[jrtc27]

WIth regards to your review: please provide your review posthaste. We
have been
working on this code submission for nearly a year via incremental PRs in
the vector-dev
branch. It's time to complete this effort.

I have my own day job to do, which this is not a part of. It's not my fault nobody else is providing detailed review like I have and that I have had to spend a lot of my time combing these changes for all the style issues. If you want these reviews to go faster, do the same yourself or find other people to help such that if and when I get round to reviewing there is less for me to do.

[Alasdair]

I went through the code. Overall I do not have any major issues, (although I am not a RISC-V vector expert so I cannot comment to that aspect!).