[vector] RFR (M): Fix ByteVector/ShortVector.shiftR

[Vladimir Ivanov]

> But for IntVector.shiftR(2)  in the test 
> http://cr.openjdk.java.net/~yzhang/vectorapi.shift/TestIntVector128.java
> 
> It is intrinsified as two instructions(vshiftcnt_0/vrsl4I_reg) instead 
> of one(vsrl4I_reg_imm).

Good catch, Yang! I should've looked more closely at generated code 
myself :-)

With the following patch I see vsrl4I_reg_imm being used:

   http://cr.openjdk.java.net/~vlivanov/panama/vector/shiftr/webrev.02/

The problem was with shared [R|L]ShiftCntV nodes: matcher can't fold 
them into multiple times unless they are marked as shared.

Best regards,
Vladimir Ivanov

> The following match rules don't work. It's the same with 
> ShortVector.shiftR(2).
> 
> x86.ad
> 
> 16140 instruct vsrl4I_imm(vecX dst, immI8 shift) %{
> 
> 16141   predicate(UseAVX == 0 && n->as_Vector()->length() == 4);
> 
> 16142   match(Set dst (URShiftVI dst (RShiftCntV shift)));
> 
> 16143   format %{ "psrld   $dst,$shift\t! logical right shift packed4I" %}
> 
> aarch64.ad
> 
> 19930 instruct vsrl4I_imm(vecX dst, vecX src, immI shift) %{
> 
> 19931   predicate(n->as_Vector()->length() == 4);
> 
> 19932   match(Set dst (URShiftVI src (RShiftCntV shift)));
> 
> 19933   ins_cost(INSN_COST);
> 
> 19934   format %{ "ushr    $dst, $src, $shift\t# vector (4S)" %}
> 
> Regards,
> Yang
> 
> ------------------------------------------------------------------------
> 
> *From:*Vladimir Ivanov <vladimir.x.ivanov at oracle.com>
> *Sent:* Friday, February 22, 2019 2:36:02 PM
> *To:* Yang Zhang (Arm Technology China); panama-dev at openjdk.java.net
> *Subject:* Re: [vector] RFR (M): Fix ByteVector/ShortVector.shiftR
> 
> Thanks, Yang.
> 
>> It looks good to me.
>> I verified the patch in our machines. Both AArch64 and x86 vector api tests are passed.
>> But I found a weird thing. I wrote a simple test http://cr.openjdk.java.net/~yzhang/vectorapi.shift/TestShortVector128.java
>> When I checked the assembly code in my test case, ShortVector.shiftR() isn't intrinsified with or without the patch. I will continue to check ByteVector/IntVector.shiftR.
> 
> Good catch!
> 
> It turned out that logical shift is disabled for short & byte since the
> implementation is used for auto-vectorization and it's incorrect to use
> URShiftBV/URShiftSV there.
> 
> How about the following?
> 
> http://cr.openjdk.java.net/~vlivanov/panama/vector/shiftr/webrev.01/
> 
> (Fixed a but in vsrl32B_avx on x86 along the way.)
> 
> Best regards,
> Vladimir Ivanov
> 
>> -----Original Message-----
>> From: panama-dev <panama-dev-bounces at openjdk.java.net> On Behalf Of Vladimir Ivanov
>> Sent: Thursday, February 21, 2019 6:44 AM
>> To: panama-dev at openjdk.java.net
>> Subject: [vector] RFR (M): Fix ByteVector/ShortVector.shiftR
>> 
>> http://cr.openjdk.java.net/~vlivanov/panama/vector/shiftr/webrev.00/
>> 
>> ByteVector/ShortVector.shiftR erroneously behaves as signed (and not
>> unsigned) shift. (There's aShiftR for the former.)
>> 
>> While fixing that, I spotted a bug in existing implementation - missing AD instructions for some shapes with constants.
>> 
>> So, the patch contains the following:
>> 
>>     * adjusts default implementation to do zero extension first: (a &
>> 0xFF) >>> (b & 7) and (a & 0xFFFF) >>> (b & 15)
>> 
>>     * adjusts C2 to do zero extension;
>> 
>>     * unifies IR shape for constant and variable shifts making LShiftCntV/RShiftCntV nodes mandatory; then extend AD instructions for constant shifts to take them into account.
>> 
>> The last point requires changes both on x86 & aarch64 side and I try to cover both.
>> 
>> Best regards,
>> Vladimir Ivanov
>> 
>