arch-riscv: Relation chain on RVV support

[aarmejach]

Gerrit relation chain on RVV support found here:
https://gem5-review.googlesource.com/c/public/gem5/+/68418/15

[aarmejach]

The test that is failing is: test-gem5-library-riscv-hello-restore-checkpoint-ALL-x86_64-opt

This test is meant to fail given the changes on the vector register file done on these commits. Should we disable it?

[powerjg]

We should probably fix it before we merge this change.

I think that we'll need to create a new checkpoint and update the test to use that new checkpoint. Give us a couple of days to look into this. @BobbyRBruce and @mkjost0

[BobbyRBruce]

I'll work on a solution to this this afternoon. It shouldn't take too long.

These checkpoint tests are quite an overhead to maintain ...

[powerjg]

@BobbyRBruce, don't forget to work on this checkpoint "fix"

[BobbyRBruce]

All you need to do to get the tests passing is change this line

gem5/configs/example/gem5_library/checkpoints/riscv-hello-restore-checkpoint.py

Line 93 in 3c6563d

checkpoint=Resource("riscv-hello-example-checkpoint-v23"),

to checkpoint=Resource("riscv-hello-example-checkpoint").

Though, in doing this, I discovered the "util/cpt_upgrader.py" did not work in this case. I logged this in Issue #106. I'm not familiar with the upgrader, but we'll probably need to fix this issue as we want to give people an easy way to upgrade their checkpoints to work with this.

[powerjg]

I think it would be good to implement the checkpoint upgrader #106 before merging this change. If you have questions on how to do this, let me know and I can try to help (I'm also not an expert 😄 ).

[aarmejach]

Thanks Bobby, I have made the modification to the config file to use the new checkpoint restore test. Let's see if tests pass now.

I can help on the cpt_upgrader if you want, I have an idea on what needs to be done there and can take a look at it on Monday.

P.S: I had way too many develop branches on diferent remotes and made a bit of a mess on my first push, think it is fixed now.

[vspetrov]

Hello, we've been looking into this patch and faced an issue with O3 CPU. The reproducer is quite large and depends on our internal library - we are working on making it smaller and plan to share it here. Meanwhile, i wanted to share our current view and check if the understanding of the problem is correct.

Basically, the reproducer looks like this:

vsetvl (type1, vl1) //First vset vl instruction that sets custom type/vl for decode
inst_squash // any instruction that will cause a pipeline squash
vse // Any vector instruction that relies on type1,vl1 durin decode
...
vsetvl (type2, vl2) // Second vset vl with different type/vl
vse // any vector instr that uses type2/vl2

What happens is: O3 CPU starts executing vsetvl №1 and saves type1/vl1 on decoder structure. Then It would create inst_squash - this can be any instruction that will cause a pipeline squash in future (it can be ld/st speculation or branch for example). Then vsetvl 2 is executed BEFORE inst_squash actually triggers pipeline squash. THis saves new type/vl on decoder. This is when pipeline is actually squashed and restarted from inst_squash. However, decoding of 1st vse instruction now uses wrong type/vl values cached on decoder from 2nd vsetvl execution.

Does that view of the problem makes sense or we missing smth?

It looks like for such scenarios we might need to maintain kind of stack data structure that would be rolled back in case of pipeline squashes (smth similar to bpu history state stack). However, it will require some mechanism to pass that info from CPU to decoder. Maybe seqNum.

[aarmejach]

The problem you describe makes sense to me. And the solution of rolling back decoder state on pipeline squashes is pobably a good way to fix it, and is related to ongoing discussion on how to better implement vsetvl instructions.

AFAIK the vsetvl instructions now stall younger instructions via a fake dependency, and there has been discussion to change vsetvl to behave like a control instruction (basically branch-like) to enable speculation around them. That is, speculate that the vl/vtype is the same as the previous and, if not, a rollback is done. Therefore, the described problem and the proposed solution can probably be used to rethink the vsetvl implementation and solve two problems in one shot.

To be honest i'd like to see this current PR merged ASAP, and try to make new issues describing problems we are seeing to fix them (like this vsetvl issue). This will allow much faster progress as now this PR is blocking multiple improvements.

I'd like some thoughts on this. Of course we should go through code review, etc., but if we merge this PR it will enable faster progress.

BTW CI tests should pass now without problems.

[powerjg]

My bad, @aarmejach . I'll make sure this is reviewed ASAP with a goal of getting it merged this week.

[vspetrov]

@aarmejach thanks. sounds good. Just out of curiosity, do you know of any actual public RTL implementation of this vector extension and how the vsetvl states are handled there?

[powerjg]

@vspetrov IIRC, the RVV standard states that vsetvl should be treated as a control instruction. Although, I don't have the exact citation for you.

From my understanding, if you're going to execute/schedule the vector instructions on an out-of-order core, the only option is to treat vsetvl as a control instruction. The performance will significantly suffer or correctness will be impacted, if you stall younger instructions after the vsetvl, at least with how many compilers generate code.

However, if you have a fully decoupled vector unit, then you would probably have more freedom in how you implement the vsetvl instruction (e.g., you could execute that instruction in the front-end).

That said, the current implementation in gem5 is going to be using the core (out-of-order or otherwise) to execute the vector instructions. So, at least in the longer term, we will likely implement vsetvl as a control instruction.

In the short term, we should probably just fail (e.g., fatal) if trying to use the O3CPU with vector extensions enabled until we implement vsetvl in a more accurate way.

[hnpl]

In case you're interested, the chapter 9 of the rvv spec has a note about control dependencies related to the vl register,

https://github.com/riscv/riscv-v-spec/blob/v1.0/v-spec.adoc#9-vector-memory-consistency-model

[powerjg]

Here's an initial review. I have a few more files I'd like to go through, but I don't expect any big hurdles.

Overall, this looks great. Let's get it in and improve on it from there!

[rogerchang23424]

I think we should drop the vecElemClass commit because it's just solve the hanging problem for vsetvl, vsetvli and vsetivli in minor CPU.

[aarmejach]

done, i removed this one and the other 2 commits specific to minor/o3.

The drawback is that now the restore-checkpoint test will fail again. This has 2 implications:

1. @BobbyRBruce could you fix the test again, basically the regs.vector_element field in the cpt now should be empty.
2. I will amend the PR util: fix cpt upgrader for rvv changes in PR #83 #115 on the cpt_upgrader now to take this change into account.

[BobbyRBruce]

done, i removed this one and the other 2 commits specific to minor/o3.

The drawback is that now the restore-checkpoint test will fail again. This has 2 implications:

1. @BobbyRBruce could you fix the test again, basically the regs.vector_element field in the cpt now should be empty.
2. I will amend the PR util: fix cpt upgrader for rvv changes in PR #83 #115 on the cpt_upgrader now to take this change into account.

I've updated the restore checkpoint. It should work with the PR as it currently stands.

[vmskv]

Hello, we've been looking into this patch and faced an issue with O3 CPU. The reproducer is quite large and depends on our internal library - we are working on making it smaller and plan to share it here. Meanwhile, i wanted to share our current view and check if the understanding of the problem is correct.

Basically, the reproducer looks like this:

vsetvl (type1, vl1) //First vset vl instruction that sets custom type/vl for decode
inst_squash // any instruction that will cause a pipeline squash
vse // Any vector instruction that relies on type1,vl1 durin decode
...
vsetvl (type2, vl2) // Second vset vl with different type/vl
vse // any vector instr that uses type2/vl2

What happens is: O3 CPU starts executing vsetvl №1 and saves type1/vl1 on decoder structure. Then It would create inst_squash - this can be any instruction that will cause a pipeline squash in future (it can be ld/st speculation or branch for example). Then vsetvl 2 is executed BEFORE inst_squash actually triggers pipeline squash. THis saves new type/vl on decoder. This is when pipeline is actually squashed and restarted from inst_squash. However, decoding of 1st vse instruction now uses wrong type/vl values cached on decoder from 2nd vsetvl execution.

Does that view of the problem makes sense or we missing smth?

It looks like for such scenarios we might need to maintain kind of stack data structure that would be rolled back in case of pipeline squashes (smth similar to bpu history state stack). However, it will require some mechanism to pass that info from CPU to decoder. Maybe seqNum.

The shareable reproducer has been prepared:
https://github.com/vmskv/gem5/tree/vmskv/rvv_bad_o3_vsetvl_execuion/riscv_o3_bad_vsetvl_execution_reproducer
In out-of-order(O3CPU) mode if vsetvl is in mispredicted branch it changes the 'vl' value in decoder and the vector instructions afterwards are decoded/executed bad after the mispredicted vsetvl squashed.

[aarmejach]

Thanks @vmskv. I think we should open an issue with the description of the problem and the reproducible, which will be the motivation to modify the current vset* implementation.

[powerjg]

I went ahead and added a couple of commits to this chain. If it's OK with you, I can simply push them here. Otherwise, you can pull them from my repo: https://github.com/powerjg/gem5/commits/pr/aarmejach/83

I fixed some style problems and I added a fatal if RVV is used with o3 or minor.

[aarmejach]

@powerjg feel free to push them here.

@BobbyRBruce a quick way to fix the restore-checkpoint test again is to run the cpt_upgrader of PR #115 over the checkpoint used in checkpoint=Resource("riscv-hello-example-checkpoint") that this PR is using.

Hopefully, this should get us close to merge this. I am going into vacation mode, but will monitor this to make sure we get it in asap.

[vmskv]

Thanks @vmskv. I think we should open an issue with the description of the problem and the reproducible, which will be the motivation to modify the current vset* implementation.

The issue is opened #144

[rogerchang23424]

Hi @powerjg,

The additional commits are LGTM. However, I think it will be better to move the enable_rvv option from RiscvDecorder to RiscvISA as it is one of isa configuration. There are two benefits of this.

1. Some of isa options riscv_type and check_alignment are put in RiscvISA. If the enable_rvv is in RiscvISA, it will be easier for user to configure.
2. The initialization of mstatus and misa CSR are depending on the enable_rvv option.

(nit) Riscv decoder can get enable_rvv option by (dynamic_cast<ISA*>(isa))->enableRvv(); .

[powerjg]

Thanks for the feedback @rogerchang23424, I took your suggestions into account.

@aarmejach, I had to force push. I hope that doesn't cause a big headache for you.

There's one more comment to address from Roger and we need to figure out the checkpoint. After that, I think we're good to go.

[rogerchang23424]

Let Jason and Bobby do final review

[aarmejach]

Sure, all comments are addressed and tests should pass.

Once a final review is done feel free to hit the button.

[powerjg]

Thanks @aarmejach for pushing this through!

[powerjg]

Thanks for the hard work everyone!