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

[Vladimir Ivanov]

Thanks a lot for detailed explanation, Yang!

So, the root problem is that channeling a value through the heap limits 
the tests we can perform on it. Right?

Another approach would be to always operate on sub-word results as 
ints/longs. But I'm fine with your proposal. Feel free to push it.

Best regards,
Vladimir Ivanov

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