RFR: 8285790: AArch64: Merge C2 NEON and SVE matching rules

[Andrew Dinn]

On Mon, 1 Aug 2022 04:06:14 GMT, Ningsheng Jian <njian at openjdk.org> wrote:

>> @shqking Thanks for the update. This looks very impressive. You have factored out the different cases very cleanly into distinct groups, including using m4 to avoid a lot of 'cookie cutter' repetition, while still providing the ability to allow easy overrides for special cases, especially cases where we want to be able to choose whether we steer the implementation towards sve or neon.
>> 
>> This change modifies a great deal of rule code and my concern is that this introduces a lot of opportunity for regressions. @theRealAph and I have both looked over the m4 and ad file rules and have not been able to spot anything that is wrong. Of course, that is no guarantee that everything is right but I am not sure we will find any more errors by reading the code. So, we are left with reliance on test coverage. Can you comment on how far you think the existing tests actually exercise the modified rules? Do you think we need to introduce more tests before committing this change? In particular
>> 
>> 1. How good is our overall coverage for Neon
>> 2. Are there rules for specific operations that you think are not well covered.
>
>> 
>> 1. How good is our overall coverage for Neon
>> 2. Are there rules for specific operations that you think are not well covered.
> 
> Thank you @adinn for the review! @shqking is taking leave, so I will try to answer the questions.
> 
> I agree that it's not easy to spot more issues on the big patch. Though @theRealELiu and I also reviewed the whole patchset several times, we still got an issue during the latest round of tests, when the option MaxVectorSize=8 is specified. Fixed in the new commit.
> 
> And yes, "Test coverage" is very important to us when we started this refactoring work. Before @shqking 's vacation, we had a review of the coverage, and here's the summary:
> 
> 1. Can all the matching rules be covered by existing JTreg test cases?
> 
> It's not an easy question to answer due to the big number of rules. Here is the conclusion based on our testing and experience.
> 
> a) Vector API provides systematic test cases to cover most of rules, except for some chain rules, e.g., 
> `// vector add reg imm (unpredicated)` rules and `MLA RELATED` rules.
> 
> b) We manually checked those chain rules, and found most of them were covered by jtreg tests as well.
> E.g.,
> `test/hotspot/jtreg/compiler/codegen/TestByteVect.java` was designed for `// vector add reg imm (unpredicated)` rules, while Vector API test cases for `broadcast(imm)` can cover `// replicate from imm` rules.
> 
> We can see only `fabd` rule is not covered so far. We've tested it with a new case - note that currently only NEON version is supported, and we plan to support SVE `fabd` after this refactoring patch. We may add the corresponding test cases in that patch.
> 
> 2. What kinds of platforms are tested?
> 
> a) Linux x86: cross compilation of arm32, ppc64, s390, riscv64. All passed.
> b) We ran Jtreg `tier1~3` on the following platforms, and no new failures.
>   - Linux x86
>   - Linux AArch64 NEON only platform
>   - Linux AArch64 NEON only platform with `-XX:MaxVectorSize=8`
>   - Linux 512-bit SVE platform
>   - Linux 256-bit SVE platform
>   - Linux 256-bit SVE platform with `-XX:UseSVE=0`
>   - Linux 256-bit SVE platform with `-XX:MaxVectorSize=8`
>   - Linux 256-bit SVE platform with `-XX:MaxVectorSize=16`
> 
> c) JMH testing
> We choose vector micro-benchmarks from panama-vector:vectorIntrinsics [1] to evaluate the performance of this
> patch. We've tested *MaxVectorTests.java cases on one 128-bit SVE platform and one NEON platform, and didn't
> see any visible regression with NEON and SVE.
> 
> 3. TODOs, before integration:
> 
> a). If possible, we will ask Oracle engineers to help run tests with this patch which may include different tests.
> b). We will continue to conduct more performance testings with NEON and different SVE vector sizes on different hardware.
> 
> [1] https://github.com/openjdk/panama-vector/tree/vectorIntrinsics/test/micro/org/openjdk/bench/jdk/incubator/vector/operation

@nsjian Hi Ningsheng! Thanks for the summary of testing status. I am happy with the current patch and tests.

I agree that both your TODOs need to be followed up before integration.

I would also ask that you obtain approval from @theRealAph before integrating.

-------------

PR: https://git.openjdk.org/jdk/pull/9346