Skip to content

Conversation

@jmawet
Copy link
Collaborator

@jmawet jmawet commented Feb 27, 2025

Adding IDL support and instruction details for Vector Extension

2/27 work in progress, do not merge to main yet. PR is for review purposes only

@jmawet jmawet requested a review from dhower-qc as a code owner February 27, 2025 19:02
@jmawet
Copy link
Collaborator Author

jmawet commented Feb 27, 2025

I used the vector_state() function from vec.idl (hope I used it right) to get the LMUL and SEW variables, but how do I use lmul_type to modify the operation of state.log2_multiplier? Would I do something like:
if (lmul_type == mul) vlmax = lmulvlensew
elseif (lmul_type == div) vlmax = (1/lmul)vlensew

?

Additionally, how should I create the CSR files for the required CSRs? Like was mentioned in our meeting, those are pretty important and should probably get done first if they don't already exist (I couldn't find any so I assume no?). And then is what I did correct in terms of using the CSRs in the operation() field, syntax wise?

@ThinkOpenly
Copy link
Collaborator

Shall we mark this as "draft" for now? (There's a button on the right.)

@jmawet
Copy link
Collaborator Author

jmawet commented Feb 27, 2025

Shall we mark this as "draft" for now? (There's a button on the right.)

I don't see the button but sure yeah

@ThinkOpenly ThinkOpenly marked this pull request as draft February 27, 2025 19:34
@ThinkOpenly
Copy link
Collaborator

Shall we mark this as "draft" for now? (There's a button on the right.)

I don't see the button but sure yeah

There's a "convert to draft" link in the "Reviewers" section for me, at least.

There's also a dropdown for the green "Create pull request" button that offers a "Create draft pull request".

Just FYI. :-)

if (xs1 < VLMAX){
CSR[vl] = uimm;
} else {
CSR[vl] = VLMAX;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to capture the two other cases listed in the spec:

  • ceil(AVL / 2) ≤ vl ≤ VLMAX if AVL < (2 * VLMAX)

If uimm/AVL is less than 2*VLMAX, then the setting of vl is implementation-dependent, subject to the above constraint. We need to add a parameter to the V extension to capture some probable behaviors, with a fallback on unpredictable(). Something like RVV_VL_WHEN_AVL_LT_DOUBLE_VLMAX, with possible values "ceil(AVL/2)", "VLMAX", "custom".

  • vl = VLMAX if AVL ≥ (2 * VLMAX)

CSR[vl] = VLMAX;
}
CSR[vtype] = zimm11;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to check for illegal vtype, and set vill if so.

@jmawet jmawet requested a review from dhower-qc March 9, 2025 00:25
@AFOliveira AFOliveira mentioned this pull request Mar 18, 2025
@dhower-qc dhower-qc linked an issue Mar 18, 2025 that may be closed by this pull request
@jmawet jmawet changed the base branch from main to vector March 26, 2025 14:34
@jmawet jmawet linked an issue Mar 26, 2025 that may be closed by this pull request
@dhower-qc dhower-qc changed the title Vector extension [WIP] Vector extension Mar 28, 2025
@jmawet
Copy link
Collaborator Author

jmawet commented Mar 30, 2025

@dhower-qc added at least the skeleton for CSRs, but have some questions on the following:

  • sw_read/write
  • priv_mode
  • type (under field)
  • a couple other misc questions (vlenb, reading, field-agnostic write)
    See my comments on the files for details

and supporting SEW=8 would need at least six bits in vl to hold the values 0-32
(VLEN=32, with LMUL=8 and SEW=8, yields VLMAX=32).
type: RO #??
reset_value: 0
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UNDEFINED_LEGAL

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how was this resolved?

This was unlinked from issues Mar 31, 2025
@jmawet jmawet linked an issue Mar 31, 2025 that may be closed by this pull request
@jmawet jmawet changed the title [WIP] Vector extension [WIP] Implement vset* and vector CSRs Apr 1, 2025
@jmawet
Copy link
Collaborator Author

jmawet commented Aug 24, 2025

Ok I think I've successfully rebased my branch, took some effort due to the directory structure changes, but I got it.

Now when I run ./do test:idl, I get the following error:

jdupac@Samwise-Server:~/git/riscv-unified-db$ ./do test:idl
Using singularity environment
Running with 1 job(s)
/home/jdupac/git/riscv-unified-db/Rakefile:222:in `block (2 levels) in <top (required)>': Missing CFG enviornment variable (RuntimeError)
        from /home/jdupac/git/riscv-unified-db/.home/.gems/ruby/3.2.0/gems/rake-13.3.0/lib/rake/task.rb:281:in `block in execute'
        from /home/jdupac/git/riscv-unified-db/.home/.gems/ruby/3.2.0/gems/rake-13.3.0/lib/rake/task.rb:281:in `each'
        from /home/jdupac/git/riscv-unified-db/.home/.gems/ruby/3.2.0/gems/rake-13.3.0/lib/rake/task.rb:281:in `execute'
        from /home/jdupac/git/riscv-unified-db/.home/.gems/ruby/3.2.0/gems/rake-13.3.0/lib/rake/task.rb:219:in `block in invoke_with_call_chain'
        from /home/jdupac/git/riscv-unified-db/.home/.gems/ruby/3.2.0/gems/rake-13.3.0/lib/rake/task.rb:199:in `synchronize'
        from /home/jdupac/git/riscv-unified-db/.home/.gems/ruby/3.2.0/gems/rake-13.3.0/lib/rake/task.rb:199:in `invoke_with_call_chain'
        from /home/jdupac/git/riscv-unified-db/.home/.gems/ruby/3.2.0/gems/rake-13.3.0/lib/rake/task.rb:188:in `invoke'
        from /home/jdupac/git/riscv-unified-db/.home/.gems/ruby/3.2.0/gems/rake-13.3.0/lib/rake/application.rb:188:in `invoke_task'
        from /home/jdupac/git/riscv-unified-db/.home/.gems/ruby/3.2.0/gems/rake-13.3.0/lib/rake/application.rb:138:in `block (2 levels) in top_level'
        from /home/jdupac/git/riscv-unified-db/.home/.gems/ruby/3.2.0/gems/rake-13.3.0/lib/rake/application.rb:138:in `each'
        from /home/jdupac/git/riscv-unified-db/.home/.gems/ruby/3.2.0/gems/rake-13.3.0/lib/rake/application.rb:138:in `block in top_level'
        from /home/jdupac/git/riscv-unified-db/.home/.gems/ruby/3.2.0/gems/rake-13.3.0/lib/rake/application.rb:147:in `run_with_threads'
        from /home/jdupac/git/riscv-unified-db/.home/.gems/ruby/3.2.0/gems/rake-13.3.0/lib/rake/application.rb:132:in `top_level'
        from -e:1:in `<main>'

Am I missing something in my setup?

@ThinkOpenly
Copy link
Collaborator

Try:

./do test:idl CFG=rv32

(or "rv64" or the other configs in /cfgs).

@jmawet
Copy link
Collaborator Author

jmawet commented Aug 26, 2025

ok... progress.
Tests are able to run, I've been able to fix some of the errors reported, but stuck at this one:

/home/jdupac/git/riscv-unified-db/tools/ruby-gems/idlc/lib/idlc/ast.rb:359:in `type_error': In file /home/jdupac/git/riscv-unified-db/gen/spec/rv32/inst/V/vsetivli.yaml (Idl::AstNode::TypeError)
On line 30
In the code:

  28: VectorState state = vector_state();
  29: XReg VLMAX = (VLEN `<< state.log2_multiplier) >> (3 + state.vsew);
  30: XReg AVL = uimm;
  31: XReg CEIL_AVL_OVER_TWO = (AVL + 1) / 2;
  

A type error occurred
  Widening shift amount must be constant (if it's not, the width of the result is unknowable).

IDL doesn't seem to like me using the widening operation that depends on the vector state (line 29).
So either we allow these "unknown widenings" in IDL, or we find a different way of getting the VLMAX value with a constant widening

@ThinkOpenly
Copy link
Collaborator

  • ceil(AVL / 2) ≤ vl ≤ VLMAX
    I created a param with possible values "ceil(AVL/2)", "VLMAX", "custom". If "custom" I set to fallback on unpredictable(). Would it maybe be better to add a second param to store the "custom" value?

What you did looks good to me.

so no need to add an additional parameter for "custom", just leave as "unpredictable" in that case?

Using this list of enums seems strange to me.

Can it be described as a function like vl(AVL,VLMAX) {...} and a validation expression that ensures the result of the function is valid? I guess we really don't support something like that currently.

@jmawet
Copy link
Collaborator Author

jmawet commented Sep 7, 2025

@dhower-qc ./do test:idl and smoke are both passing now for cfgs rv32/64. Had a decent amount of compilation errors to clean up, but it's all fixed now.
VLEN and ELEN are added to the v ext parameters as well

all that's missing now is 1 more review and the instruction tests we discussed this week, and we're ready to merge!

Comment on lines 71 to 72
sw_write(csr_value): |
return csr_value.VALUE & (VLEN - 1);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The spec says:

The vstart CSR is defined to have only enough writable bits to hold the largest element index (one less than the maximum VLMAX).

VLEN units are bits, but vstart units are elements.

This means that:

  • The defined number of bits the largest to hold the biggest VLMAX, which happens when LMUL is maxed out and SEW is minimum. (LMUL_MAX * VLEN / SEW_MIN). Assuming both LMUL_MAX and SEW_MIN are 8, the biggest VLMAX is 256.
  • When SEW/LMUL have different settings, there will be 'reserved' bits in vstart. It is undefined what happens when you write non-zero into the reserved bits, but there is a recommendation that it takes a takes a trap. This is a parameter that needs to be added.

@jmawet jmawet requested a review from dhower-qc September 15, 2025 00:54
# SPDX-SnippetBegin
# SPDX-FileCopyrightText: 2017-2025 Contributors to the RISCV Sail Model <https://github.com/riscv/sail-riscv/blob/master/LICENCE>
# SPDX-License-Identifier: BSD-2-Clause
sail(): |
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where did this come from? If you wrote it, I admire the effort. We probably want to treat Sail content as a unit, and find a programmatic way to bring it all from the canonical Sail code at once (or continually, but not manually). Reluctantly, I suggest that you remove this code and leave the sail() method empty here.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still needs attention.

jmawet and others added 6 commits September 21, 2025 11:49
Co-authored-by: Paul Clarke <[email protected]>
Signed-off-by: Jennifer Dupaquier <[email protected]>
Co-authored-by: Paul Clarke <[email protected]>
Signed-off-by: Jennifer Dupaquier <[email protected]>
Co-authored-by: Paul Clarke <[email protected]>
Signed-off-by: Jennifer Dupaquier <[email protected]>
Co-authored-by: Paul Clarke <[email protected]>
Signed-off-by: Jennifer Dupaquier <[email protected]>
Co-authored-by: Paul Clarke <[email protected]>
Signed-off-by: Jennifer Dupaquier <[email protected]>
@jmawet jmawet requested a review from ThinkOpenly September 21, 2025 19:07
$schema: "csr_schema.json#"
kind: csr
name: vxsat
long_name: Vector Fixed-Point Saturate Flag
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
long_name: Vector Fixed-Point Saturate Flag
long_name: Vector Fixed-Point Saturation Flag

$schema: "csr_schema.json#"
kind: csr
name: vcsr
long_name: Vector Control and Status Register
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
long_name: Vector Control and Status Register
long_name: Vector Control and Status

I'm basing this on the spec's convention to call the section for the register "[long_name] (mnemonic) Register", like:

Vector Type (vtype) Register
Vector Length (vl) Register
Vector Byte Length (vlenb) Register

and so on. vcsr is listed thus:

Vector Control and Status (vcsr) Register

@henrikg-qc henrikg-qc merged commit 49b0d4d into riscv-software-src:vector Sep 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement vset* and vector CSRs

5 participants