Re: Re: [PATCH] RISC-V: Add vm* mask C api tests

[Kito Cheng <kito.cheng@gmail.com>]

TL;DR: I think most parts of the test could be added by generator
instead of adding those test cases directly, we gonna stop putting all
API testing testcase.


RISC-V Vector intrinsic is not implement through the *.def file way,
it's using same approach as SVE's intrinsic,
create and register by C files, the reason is RISC-V vector has a huge
set for the intrinsic function:
about ~80k function for different combinations.

So that result we have so huge testcase set, and let me break down that

There are several kinds for those testcase:
1. vsetvli insertion pass testing, which is a highly customized mode
switching pass
2. Code gen test: testing our move pattern and generated code has
satisfied the RISC-V vector ISA constraint.
3. Intrinsic API testing: test the C intrinsic has right interface and
generated expected instruction

---

FIrst part has 375 testcase in `testsuite/gcc.target/riscv/rvv/vsetvl`
which is important and ~16M
but one potential issue is that is highly code gen sensitive, we've
added many long scan-assembly in the test file,
it's not ideal and we plan to implement a builtin verifier inside GCC
instead of lots of long scan-assembly,
This is planned for this year, but will happen after GCC 13 release.

---

Second part is also important, and only 300~400 files, so I think this
part should just keep as it is.

---

The last part is the most huge part in the testcases (~3000 files so
far), and I think we should consider removing this part from the GCC
testsuite,
since we have a standard one[1] from the RISC-V international.

So my thought is we stop putting further intrinsic API testing now,
and use the external one,
and evaluate the effort and benefit of implementing a test generator
inside GCC in future (after GCC 13 release).

[1] https://github.com/riscv-non-isa/rvv-intrinsic-doc/blob/master/auto-generated/api-testing/


On Thu, Feb 16, 2023 at 6:32 PM juzhe.zhong@rivai.ai
<juzhe.zhong@rivai.ai> wrote:
>
> Well, I think the best solution:
> 1. Remove all intrinsic test that I already commited.
> 2. Then, embed test-generator for this intrinsic unit-test.
> 3. Call  test-generator during regression and test them.
> 4. Remove the testcases generated by the test-generator after regression.
>
> Not sure whether you aggree with me.
>
> The test-generator I used is generating the testcase by reading the rvv-intrinsic document directly and generate the testcases.
> That means I need to commit test-generator and rvv-intrinsic document both.
> I don't think my test-generator is good to commit.
>
> I believe Kito has the mature and better test-generator (much better than mine) to commit since rvv-intrinsic doc is their work.
>
> As long as we can make kito's test-generator embedded into GCC regression, this issue will be fixed. And I believe we can fix it soon.
>
> So...Let's wait for kito.
>
>
> juzhe.zhong@rivai.ai
>
> From: Jakub Jelinek
> Date: 2023-02-16 18:20
> To: juzhe.zhong@rivai.ai
> CC: gcc-patches; kito.cheng; jeffreyalaw
> Subject: Re: Re: [PATCH] RISC-V: Add vm* mask C api tests
> On Thu, Feb 16, 2023 at 05:53:48PM +0800, juzhe.zhong@rivai.ai wrote:
> > Thanks for reporting this. I think may be we can make reduce tests into 1/3.
> > For example:
> > We have:
> > * gcc.target/riscv/rvv/base/vmand_mm-1.c: New test.
> > * gcc.target/riscv/rvv/base/vmand_mm-2.c: New test.
> > * gcc.target/riscv/rvv/base/vmand_mm-3.c: New test.
> >
> > Maybe we can reduce it into one test:
> > vmand_mm.c only.
> >
> > I will improve and reduce all intrinsic tests like this soon (I almost done all intrinsic in this week, next week I will do this soon).
> >
> > RVV intrinsics are really huge, this is the document:
> > https://github.com/riscv-non-isa/rvv-intrinsic-doc/tree/master/auto-generated
> >
> > The testcases are directly come from LLVM (We just add assembler check into the test), they also have this amount of testcases and the just recently change them:
> > https://reviews.llvm.org/D142697
> > https://reviews.llvm.org/D142644
> >
> > Take a look at the changing LLVM patch, I am aggree with you ,the LLVM patch is quite huge and not easy to maintain.
>
> Yeah, LLVM does this all the time, their unit-tests where they embed e.g.
> matchers for IL in huge tests.
>
> I just think the way they are doing this is a very bad idea.
> If say one writes some C/C++ test, compile it, some helper program
> adds the IL into comments in the test then again any time you want to
> adjust something in the compiler that affects those tests, you need to
> regenerate them.  Is the generator included somewhere, or does every
> user write his own tooling to do that?  Anyway, if the solution is
> regenerate the IL, the test lost quite lot of its meaning, because
> when changing thousands of tests and regenerating the IL for all of them,
> one can hardly expect to carefully examine the changes to all those tests
> whether everything was intended.
>
> In GCC we have far fewer such unit-tests and big parts of the testsuite
> are testing everything from parsing through assembly through linking through
> runtime.  In my experience over the years, many such tests can discover even
> bugs completely unrelated to the original reason why a test has been added.
>
> If they have some generator in LLVM for these riscv tests, even worse,
> there is another step for LLVM generator regenerates them on the LLVM side
> and somebody needs to reimport them into GCC and regenerate the
> scan-assembler regexps.
>
> riscv already uses what various other GCC backends use for builtins and
> intrinsics, various *.def files from which the actual support is created.
> So, can't we use the same files + something on top of that to have the
> testsuite coverage, or if it should be independent from it, at least
> have something similar which would describe intrinsic that should be tested,
> iterate over such and such types for which arguments and how to come up with
> the expected emitted code.
> So, rather than reducing the tests into 1/3, try to reduce them to one
> line per intrinsic or something of that scale.
>
> Jakub
>
>