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

[Andrew Dinn]

On Wed, 27 Jul 2022 04:14:08 GMT, Hao Sun <haosun at openjdk.org> wrote:

>>> @theRealAph Thanks for your comment. Yes. There are still duplicate code. I can easily list several ones, such as the reduce-and/or/xor, vector shift ops and several reg with imm rules. We're open to keep m4 file.
>>> 
>>> But I would suggest that we may put our attention firstly on 1) our implementation on generic vector registers and 2) the merged rules (in particular those we share the codegen for NEON only platform and 128-bit vector ops on SVE platform). After that we may discuss whether to use m4 file and how to implement it if needed.
>> 
>> We can do both: there's no sense in which one excludes the other, and we have time.
>> 
>> However, just putting aside for a moment the lack of useful abstraction mechanisms, I note that there's a lot of code like this:
>> 
>> 
>>     if (length_in_bytes <= 16) {
>>       // ... Neon
>>     } else {
>>       assert(UseSVE > 0, "must be sve");
>>       // ... SVE
>>     }
>> 
>> 
>> which is to say, there's an implicit assumption that if an operation can be done with Neon it will be, and SVE will only be used if not. What is the justification for that assumption?
>
> @theRealAph @adinn 
> Thanks for your reviews!
> I have updated the patch based on the review comments. Would you mind taking another look? Thanks!

@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.

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

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