Re: Re: [PATCH] RISC-V: Add vm* mask C api tests
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.zh...@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.zh...@rivai.ai > > From: Jakub Jelinek > Date: 2023-02-16 18:20 > To: juzhe.zh...@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.zh...@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
Re: Re: [PATCH] RISC-V: Add vm* mask C api tests
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.zh...@rivai.ai From: Jakub Jelinek Date: 2023-02-16 18:20 To: juzhe.zh...@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.zh...@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
Re: Re: [PATCH] RISC-V: Add vm* mask C api tests
On Thu, Feb 16, 2023 at 05:53:48PM +0800, juzhe.zh...@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
Re: Re: [PATCH] RISC-V: Add vm* mask C api tests
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. So.. I think I can reduce the tests into 1/3 of them in the next. But it's still very big (you can take a look at LLVM). Let's see whether kito has more comments about it. juzhe.zh...@rivai.ai From: Jakub Jelinek Date: 2023-02-16 17:38 To: juzhe.zhong CC: gcc-patches; kito.cheng; Jeff Law Subject: Re: [PATCH] RISC-V: Add vm* mask C api tests Hi! I see in the past few weeks you've added huge amounts of these tests du -shc *.target/riscv/*/ 34M gcc.target/riscv/rvv/ 28M g++.target/riscv/rvv/ 61M total and new are coming (nothing at all at this year's start). This is far larger than tests of any other architecture (i386 has 35M total, aarch64 31M total, arm 17M total, powerpc 12M total, everything else is even much smaller) but for the other architectures it has been decades of testsuite coverage for features added over the years. Rather than looking purely at size, I'm more worried about the content of the tests. Usually target testsuites include runtime tests whether particular intrinsics etc. behave correctly at runtime, plus some compile tests that they can be compiled with occassional scan-assembler* to mention a particular instruction appears, but in these cases the scan-assembler* covers the entire (albeit small) functions, which makes it IMHO a maintainance nightmare whenever one wants to change something important in the compiler. Take e.g. the recent Andreas Schwab's change to make -fasynchronous-unwind-tables the default on riscv, even that change required quite a few changes. My worry is that with these kind of tests changes like that will become much harder and some people will simply decide not to do such changes because having to adjust tens of thousands of tests even with some scripting would be a nightmare. Can't we do better than this? E.g. what is the difference between gcc.target/riscv/rvv/ and g++.target/riscv/rvv/ tests? Are the APIs so different between C and C++ that it needs to be tested twice? Even if so, we have the concept of c-c++-common tests, we could add c-c++-common.target and make riscv.exp handle it similarly to how e.g. C and C++ dg.exp handles those. How do you create these tests? If you use some generator for them, wouldn't it be better to include the generator in the testsuite and generate them on the fly? We already have a precedent for that, e.g. the gcc/testsuite/g*.dg/compat/struct-layout-1.exp testsuite has a generator program written in C that creates tests on the fly. Now, using something like that would have 2 advantages, it would be much easier for maintainance, if you do some global change in the compiler that affects those tests, just adjust a few spots in the generator instead of tweaking currently 6000 tests and counting. Even if you aren't using a generator to write these tests (that would be a lot of work then!), a question is if it couldn't be done by one, have say some file like gcc has *.def files all around to describe what you want to test and something that generates those. Just wanted to chime in before we have 10 times more of such tests and it will be too late to adjust... On Thu, Feb 16, 2023 at 11:36:19AM +0800, juzhe.zh...@rivai.ai wrote: > From: Ju-Zhe Zhong > > gcc/testsuite/ChangeLog: > > * 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. > * gcc.target/riscv/rvv/base/vmandn_mm-1.c: New test. > * gcc.target/riscv/rvv/base/vmandn_mm-2.c: New test. > * gcc.target/riscv/rvv/base/vmandn_mm-3.c: New test. > * gcc.target/riscv/rvv/base/vmclr_m_m-1.c: New test. > * gcc.target/riscv/rvv/base/vmclr_m_m-2.c: New test. > * gcc.target/riscv/rvv/base/vmclr_m_m-3.c: New test. > * gcc.target/riscv/rvv/base/vmmv_m_m-1.c: New test. > * gcc.target/riscv/rvv/base/vmmv_m_m-2.c: New test. > * gcc.target/riscv/rvv/base/vmmv_m_m-3.c: New test. > * gcc.target/riscv/rvv/base/vmnand_mm-1.c: New test. >
