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

Vector Extension #35

Open
wants to merge 222 commits into
base: stage3
Choose a base branch
from
Open

Vector Extension #35

wants to merge 222 commits into from

Conversation

1fahadaloufi
Copy link

Implement zve32x subset of the RISC-V vector extension without fixed point instructions. Documentation about this vector extension implementation is in doc/src/rv32v.

OxyMagnesium and others added 30 commits November 3, 2023 22:40
(cherry picked from commit 27ae013)
@maxmichalec
Copy link

maxmichalec commented Aug 9, 2024

TODO before merge:

  • Replace *_wide signals in vector mem stage / coalescer
  • Add atomics support to vector load-store controller

Copy link

@devins2518 devins2518 left a comment

Choose a reason for hiding this comment

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

Looks good so far. Once the coalescer is fixed and there's a bit of cleanup, I think its ready to merge

verification/asm-tests/RV32I/add.S Outdated Show resolved Hide resolved
verification/self-tests/RV32I/add.S Outdated Show resolved Hide resolved
verification/self-tests/RV32I/simple_add.S Outdated Show resolved Hide resolved

Choose a reason for hiding this comment

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

This is more of a general comment on the rv32v asm files, a majority of the benchmarking files is usually the test data from what I've seen, is it reused enough that the common data can be pulled out into a helper file that each benchmark file includes?

Choose a reason for hiding this comment

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

What's the purpose of these hidden files?

source_code/rv32v/source/rv32v_uop_gen.sv Outdated Show resolved Hide resolved
source_code/rv32v/source/rv32v_ex_datapath.sv Outdated Show resolved Hide resolved

Choose a reason for hiding this comment

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

Is it possible to use the stage3 fetch so that any fixes can be shared by both

Choose a reason for hiding this comment

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

Same with the hazard unit, could we pull it out to be "pipeline agnostic" and enable certain parts depending on the pipeline config

source_code/pipelines/stage4/source/stage4.sv Outdated Show resolved Hide resolved
@devins2518
Copy link

devins2518 commented Aug 9, 2024

Also are there any defines for the tests that I can change to have more test coverage?

@maxmichalec
Copy link

Tests are passing with *_wide signals removed. Also fixed passthrough reads bug (@devins2518 check that the fix is OK). For cacheable blocks, the whole block can be written to if the address is block-aligned, otherwise only one block is written to/read from at a time. Non-cacheable, potentially non-idempotent, regions only deal with word-wide accesses (the vector memory controller is aware of this and every access is serialized and in-order). This wouldn't be too difficult to widen in the future once the AHB bus is widened.

Will cleanup next and we should add tests for 2 cores running vector code.

Copy link

@devins2518 devins2518 left a comment

Choose a reason for hiding this comment

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

Looks good so far, nice catch on the pass through bug. The write strategy makes sense and seems to be pretty simple implementation wise.

Choose a reason for hiding this comment

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

Nice catch, this looks good. The conditional probably isn't needed but it should be fine

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.

5 participants