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

RISC-V RVV Bad execution of riscv rvv vss instruction #594

Closed
vmskv opened this issue Nov 23, 2023 · 4 comments
Closed

RISC-V RVV Bad execution of riscv rvv vss instruction #594

vmskv opened this issue Nov 23, 2023 · 4 comments

Comments

@vmskv
Copy link

vmskv commented Nov 23, 2023

Describe the bug
RVV vsseg2e32 instruction doesn't work properly.

Affects version
Found in commit 3896673 (develop)
Date: Sat Nov 18

To Reproduce
The reproducer is in https://github.com/vmskv/gem5/tree/vmskv/bad_interleave_reproducer
The steps to reproduce are in
README_assembler.txt
or in
README_intrinsics.txt

Expected behavior
Instruction should interleave the elements of input vectors

expected:

           inps/out         
rez : i = 0 : 1 5
rez : i = 1 : 2 1
rez : i = 2 : 3 6
rez : i = 3 : 4 2
rez : i = 4 : 5 7
rez : i = 5 : 6 3
rez : i = 6 : 7 8
rez : i = 7 : 8 4

but returns 0s

rez : i = 0 : 1 0
rez : i = 1 : 2 0
rez : i = 2 : 3 0
rez : i = 3 : 4 0
rez : i = 4 : 5 0
rez : i = 5 : 6 0
rez : i = 6 : 7 0
rez : i = 7 : 8 0

Host Operating System
Ubuntu 20.04

Guest ISA
RISC-V.

@hnpl
Copy link
Contributor

hnpl commented Nov 26, 2023

Thanks for pointing this out. From what I see In the gem5 VSStride macro-op template (

def template VsStrideConstructor {{
%(class_name)s::%(class_name)s(ExtMachInst _machInst, uint32_t _vlen)
: %(base_class)s("%(mnemonic)s", _machInst, %(op_class)s, _vlen)
{
%(set_reg_idx_arr)s;
%(constructor)s;
const int32_t num_elems_per_vreg = vlen / width_EEW(_machInst.width);
int32_t remaining_vl = this->vl;
// Num of elems in one vreg
int32_t micro_vl = std::min(remaining_vl, num_elems_per_vreg);
StaticInstPtr microop;
if (micro_vl == 0) {
microop = new VectorNopMicroInst(_machInst);
this->microops.push_back(microop);
}
for (int i = 0; micro_vl > 0; ++i) {
for (int j = 0; j < micro_vl; ++j) {
microop = new %(class_name)sMicro(machInst, i, j, micro_vl);
microop->setFlag(IsDelayedCommit);
microop->setFlag(IsStore);
this->microops.push_back(microop);
}
remaining_vl -= num_elems_per_vreg;
micro_vl = std::min(remaining_vl, num_elems_per_vreg);
}
this->microops.front()->setFlag(IsFirstMicroop);
this->microops.back()->setFlag(IsLastMicroop);
this->flags[IsVector] = true;
}
}};
) and the corresponding generated ISA code (build/RISCV/arch/riscv/generated/exec-ns.cc.inc), the VSS (vector strided-store) macro instructions do not take into account NFIELDS; so I don't think the segmented VsStride instructions with NFIELDS != 0 are supported.

@ivanfv
Copy link

ivanfv commented Nov 28, 2023

Hi,

I guess this is expected, since as far as I am aware unit-stride segmented loads/stores are not implemented yet. Their encoding only differ from vle/vse in NFIELDS, which is not currently checked in the decoder. Because of that, they are probably being interpreted as vle/vse ones (which makes sense with the results you are getting, only first field is right).

I am currently working on the implementation of these instructions (#382). While I expect to have them soon, it is still work in progress.

-Ivan

@ivanaamit
Copy link
Contributor

Hi @ivanfv, could you please let us know if this issue should be closed or if there is a real bug? Thank you.

@ivanfv
Copy link

ivanfv commented Apr 16, 2024

Hi @ivanaamit, yes, I think this issue should be closed since vector segmented store instructions were added with PR #913

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

No branches or pull requests

4 participants