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

[Yang Zhang (Arm Technology China)]

Hi Vladimir



I use your patch and ShortVector.shiftR is intrinsified now.



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



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
>