[vector api] RFR: Fine-tune test cases for reduction operations

[Yang Zhang (Arm Technology China)]

Hi Vladimir



The IR level is exactly correct. The issue of current test is as followed.


I write a simple test case: http://cr.openjdk.java.net/~yzhang/vectorapi.test/testcase/TestShortVector128.java


ideal graph of vectMaxAll2 which checks result of one vector, just like current test. http://cr.openjdk.java.net/~yzhang/vectorapi.test/testcase/ig_vectMaxAll2.png


           ConI->

LoadVector-> MaxReductionV -> StoreC


This test makes sure that result of one vector is correct.



ideal graph of vectMaxAll1 which checks result of multiple vectors. http://cr.openjdk.java.net/~yzhang/vectorapi.test/testcase/ig_vectMaxAll1.png


           ConI->

LoadVector-> MaxReductionV -> CmpI -> other operations
                                          Phi  ->


MaxReductionV is implemented by multiple instructions and the intermediate register value

may be reused in next step such as CmpI. This test can make sure the intermediate register value is correct too.


Assume input {1, 2, 3, 4, -1, -2, -3, -4} to ShortVector64:

For vectMaxAll2, the result is {4, -1}

For vectMaxAll1, the result should be CmpI (4, -1) -> 4.

But If I use umov(unsigned move) to transfer (-1, 0xFFFF) from vector register carelessly, just like reduce_add4S, the result will be CmpI(0x4, 0xFFFF) -> 0xFFFF(-1).


So that I think we need to check the result of one vector and multiple vectors to improve test coverage.


Regards

Yang

________________________________
From: Vladimir Ivanov <vladimir.x.ivanov at oracle.com>
Sent: Thursday, February 14, 2019 6:18 AM
To: Yang Zhang (Arm Technology China); 'panama-dev at openjdk.java.net'
Subject: Re: [vector api] RFR: Fine-tune test cases for reduction operations


> The following is an example on AArch64 platform. Without the patch, the test is passed . But it is failed with this patch.
>
> I make a little change to this instruction:
> instruct reduce_max8S(iRegINoSp dst, iRegIorL2I src1, vecX src2, vecX tmp, rFlagsReg cr) %{
>    format %{ "smaxv  $tmp, T8H, $src2\n\t"
>              "umov  $dst, $tmp, H, 0\n\t"----------------> smov should be used. because umov will recognize a negative as a great positive.
>              "cmpw  $dst, $src1\n\t"
>              "cselw $dst, $dst, $src1 gt\t max reduction8S" %}
>
> %}
> smov: Signed Move vector element to general-purpose register.
> umov: Move vector element to general-purpose register.
>
> The test is passed with original test. The error is missed, because src1 is always Short.MIN_VALUE, reduce_max8S could give the correct result of one vector.
> test Short128VectorTests.maxAllShort128VectorTests(short[i * 5]): success

Can you, please, elaborate why there's a difference on IR level w.r.t.
src1? (Sorry, haven't had time to explore that myself.)

bool LibraryCallKit::inline_vector_reduction() {
...
   Node* init = ReductionNode::make_reduction_input(_gvn, opc, elem_bt);
   Node* rn = gvn().transform(ReductionNode::make(opc, NULL, init, opd,
elem_bt));
...

What I'm seeing in library_call.cpp is it always initializes the value
with the same constant. Are there any transformations happening after
that? If yes, then where do they happen?

Best regards,
Vladimir Ivanov

>
> The test is failed with new test. The error is caught.
> test Short128VectorTests.maxAllShort128VectorTests(short[i * 5]): failure
> java.lang.AssertionError: Final result is incorrect! expected [32765] but found [-25541]
>
> So that I think checking both the final and intermediate result is better.
> I update the patch a little. Please check it.
> http://cr.openjdk.java.net/~yzhang/vectorapi.test/webrev.01/
>
> Regards,
> Yang
>
> -----Original Message-----
> From: Vladimir Ivanov <vladimir.x.ivanov at oracle.com>
> Sent: Tuesday, February 12, 2019 9:37 AM
> To: Yang Zhang (Arm Technology China) <Yang.Zhang at arm.com>; 'panama-dev at openjdk.java.net' <panama-dev at openjdk.java.net>
> Subject: Re: [vector api] RFR: Fine-tune test cases for reduction operations
>
> Though I like the changes you propose, I don't see how they improve test coverage. It implies C2 does the following transformation, but I don't think that's the case (yet):
>
>     AddI i (AddReductionVI (ConI 0) v) => AddReductionVI i v
>
> Am I missing something?
>
> Best regards,
> Vladimir Ivanov
>
> On 11/02/2019 00:25, Yang Zhang (Arm Technology China) wrote:
>> Hi
>>
>> Currently, test cases of reduction operations only check the intermediate result (the result of one vector), not final result ( the result of one array). So that for following examples, src1 is always initial value without this patch.
>> instruct rsadd2I_reduction_reg(rRegI dst, rRegI src1, vecD src2, vecD
>> tmp, vecD tmp2) %{  ----------------------X86 instruct
>> reduce_add2I(iRegINoSp dst, iRegIorL2I src1, vecD src2, iRegINoSp tmp, iRegINoSp tmp2) ------------------AArch64 To cover more cases, I add test cases that check the final result of reduction operations. Some indentation errors are also fixed.
>> With this patch, all Vector API tests are passed.
>>
>> Could you please help to review this patch?
>> http://cr.openjdk.java.net/~yzhang/vectorapi.test/webrev.00/
>>
>> Regards,
>> Yang
>>