Re: [Qemu-devel] [PATCH] RISCV: support riscv vector extension 0.7.1

[Alistair Francis]

On Thu, Aug 29, 2019 at 5:05 AM liuzhiwei <address@hidden> wrote:
>
> On 2019/8/29 上午5:34, Alistair Francis wrote:
> > On Wed, Aug 28, 2019 at 12:04 AM liuzhiwei <address@hidden> wrote:
> >> Change-Id: I3cf891bc400713b95f47ecca82b1bf773f3dcb25
> >> Signed-off-by: liuzhiwei <address@hidden>
> >> ---
> >>   fpu/softfloat.c                         |   119 +
> >>   include/fpu/softfloat.h                 |     4 +
> >>   linux-user/riscv/cpu_loop.c             |     8 +-
> >>   target/riscv/Makefile.objs              |     2 +-
> >>   target/riscv/cpu.h                      |    30 +
> >>   target/riscv/cpu_bits.h                 |    15 +
> >>   target/riscv/cpu_helper.c               |     7 +
> >>   target/riscv/csr.c                      |    65 +-
> >>   target/riscv/helper.h                   |   354 +
> >>   target/riscv/insn32.decode              |   374 +-
> >>   target/riscv/insn_trans/trans_rvv.inc.c |   484 +
> >>   target/riscv/translate.c                |     1 +
> >>   target/riscv/vector_helper.c            | 26563 
> >> ++++++++++++++++++++++++++++++
> >>   13 files changed, 28017 insertions(+), 9 deletions(-)
> >>   create mode 100644 target/riscv/insn_trans/trans_rvv.inc.c
> >>   create mode 100644 target/riscv/vector_helper.c
> >>
> > Hello,
> >
> > Thanks for the patch!
> >
> > As others have pointed out you will need to split the patch up into
> > multiple smaller patches, otherwise it is too hard to review almost
> > 30,000 lines of code.
>
> Hi, Alistair
>
> I'm so sorry for the inconvenience. It will be a patch set with a cover
> letter in V2.

No worries.

>
> > Can you also include a cover letter with your patch series describing
> > how you are testing this? AFAIK vector extension support isn't in any
> > compiler so I'm assuming you are handwriting the assembly or have
> > toolchain patches. Either way it will help if you can share that so
> > others can test your implementation.
>
> Yes, it's handwriting assembly. The assembler in Binutils has support
> Vector extension.  First define an function test_vadd_vv_8 in assembly
> and then it can be called from a C program.
>
> The function is something like
>
> /* vadd.vv */
> TEST_FUNC(test_vadd_vv_8)
>          vsetvli        t1, x0, e8, m2
>          vlb.v           v6, (a4)
>          vsb.v           v6, (a3)
>          vsetvli        t1, a0, e8, m2
>          vlb.v           v0, (a1)
>          vlb.v           v2, (a2)
>          vadd.vv     v4, v0, v2
>          vsb.v          v4, (a3)
> ret
>          .size   test_vadd_vv_8, .-test_vadd_vv_8

If possible it might be worth releasing the code that you are using for testing.

>
> It takes more time to test than to implement the instructions. Maybe
> there is some better test method or some forced test cases in QEMU.
> Could you give me some advice for testing?

Richard's idea of risu seems like a good option.

Thinking about it a bit more we are going to have other extensions in
the future that will need assembly testing so setting up a test
framework seems like a good idea. I am happy to help try and get this
going as well.

Alistair

>
> Best Regards,
>
> Zhiwei
>
> > Alex and Richard have kindly started the review. Once you have
> > addressed their comments and split this patch up into smaller patches
> > you can send a v2 and we can go from there.
> >
> > Once again thanks for doing this implementation for QEMU!
> >
> > Alistair
> >