RFR: 8289604: compiler/vectorapi/VectorLogicalOpIdentityTest.java failed on x86 AVX1 system

[Xiaohong Gong]

On Tue, 5 Jul 2022 19:48:41 GMT, Vladimir Kozlov <kvn at openjdk.org> wrote:

>> The sub-test "`testMaskAndZero()`" failed on x86 systems when
>> `UseAVX=1` with the IR check failure:
>> 
>>   - counts: Graph contains wrong number of nodes:
>>      * Regex 1: (\\d+(\\s){2}(StoreVector.*)+(\\s){2}===.*)
>>         - Failed comparison: [found] 0 >= 1 [given]
>>         - No nodes matched!
>> 
>> The root cause is the `VectorMask.fromArray/intoArray` APIs
>> are not intrinsified when "`UseAVX=1`" for long type vectors
>> with following reasons:
>>  1) The system supported max vector size is 128 bits for
>> integral vector operations when "`UseAVX=1`".
>>  2) The match rule of `VectorLoadMaskNode/VectorStoreMaskNode`
>> are not supported for vectors with 2 elements (see [1]).
>> 
>> Note that `VectorMask.fromArray()` needs to be intrinsified
>> with "`LoadVector+VectorLoadMask`". And `VectorMask.intoArray()`
>> needs to be intrinsified with "`VectorStoreMask+StoreVector`".
>> Either "`VectorStoreMask`" or "`StoreVector`" not supported by the
>> compiler backend will forbit the relative API intrinsification.
>> 
>> Replacing the vector type from Long to other integral types
>> in the test case can fix the issue.
>> 
>> [1] https://github.com/openjdk/jdk/blob/master/src/hotspot/cpu/x86/x86.ad#L1861
>
> Including @chhagedorn for discussion.
> 
> I was wondering why our testing with `UseAVX=1` did not catch this issue (test passed).
> But then I remember that IR framework skip testing if such flag is used by testing. It is not on whitelist:
> https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/compiler/lib/ir_framework/TestFramework.java#L104
> 
> We are adding more IR framework testing for vectors. I think it is important to test them with different CPU's features **if needed**.
> 
> https://github.com/openjdk/jdk/pull/8999 added filter to run some sub-tests if CPU's feature is present/absent. But it relies on testing infrastructure to be executed on corresponding machine. It is not reliable (how many machines left with only AVX1).
> 
> I suggest to allow a test add flags to whitelist with which it allows to run. It (together with #8999 and `@requires`) will allow test's author to specify range of CPU features which can be used for test and make sure they will be run.

Thanks for looking at this fix @vnkozlov !

> Including @chhagedorn for discussion.
> 
> I was wondering why our testing with `UseAVX=1` did not catch this issue (test passed). But then I remember that IR framework skip testing if such flag is used by testing. It is not on whitelist: https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/compiler/lib/ir_framework/TestFramework.java#L104

Right, testing with such vm options won't trigger the IR testing! 

> 
> We are adding more IR framework testing for vectors. I think it is important to test them with different CPU's features **if needed**.
> 
> #8999 added filter to run some sub-tests if CPU's feature is present/absent. But it relies on testing infrastructure to be executed on corresponding machine. It is not reliable (how many machines left with only AVX1).
> 
> I suggest to allow a test add flags to whitelist with which it allows to run. It (together with #8999 and `@requires`) will allow test's author to specify range of CPU features which can be used for test and make sure they will be run.

Totally agree! #8999 adds the possibility to limit the test for specific CPU feature, which is convenient for the IR tests that relies on the CPU feature. Please also see one of the tests in the `VectorLogicalOpIdentityTest.java` here: https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/compiler/vectorapi/VectorLogicalOpIdentityTest.java#L160

But for the check of common IRs which is not the CPU specific ones, testing with different vm options are needed. And we cannot use `ApplyIfxxx` for the architecture specific options such as `UseAVX` or `UseSVE` in common tests (i.e. using "UseSVE" on x64 systems makes issue). So I agree that adding the needed flags to whitelist is better.

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

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