RFR(S): 8194256 - AARCH64: SIMD shift instructions are incorrectly encoded

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR(S): 8194256 - AARCH64: SIMD shift instructions are incorrectly encoded

Dmitrij Pochepko-2
Hi all,

please review small patch for 8194256 - AARCH64: SIMD shift instructions
are incorrectly encoded

I've noticed SIMD shift instructions wrong encoding when trying to use
it. An intrinsic I was working on, generated incorrect assembly code
with wrong shift value.

Existing code just copy "shift" bits into instruction bits(immh:immb),
however, according to spec, it should be encoded as follows:


SIMD type <T>:

8B when immh = 0001 , Q = 0
16B when immh = 0001 , Q = 1
4H when immh = 001x , Q = 0
8H when immh = 001x , Q = 1
2S when immh = 01xx , Q = 0
4S when immh = 01xx , Q = 1
2D when immh = 1xxx , Q = 1


<shift> is encoded as follows:


for ushr and sshr:

(16-UInt(immh:immb)) when immh = 0001
(32-UInt(immh:immb)) when immh = 001x
(64-UInt(immh:immb)) when immh = 01xx
(128-UInt(immh:immb)) when immh = 1xxx


for shl:

(UInt(immh:immb)-8) when immh = 0001
(UInt(immh:immb)-16) when immh = 001x
(UInt(immh:immb)-32) when immh = 01xx
(UInt(immh:immb)-64) when immh = 1xxx


So, I've modified respective instruction generation code.


webrev: http://cr.openjdk.java.net/~dpochepk/8194256/webrev.01/

CR: https://bugs.openjdk.java.net/browse/JDK-8194256


I've checked this patch by generating intrinsic which use these
instructions and verified assembly code.


Thanks,

Dmitrij

Reply | Threaded
Open this post in threaded view
|

Re: [aarch64-port-dev ] RFR(S): 8194256 - AARCH64: SIMD shift instructions are incorrectly encoded

Andrew Haley
On 28/12/17 14:03, Dmitrij Pochepko wrote:
>
> webrev: http://cr.openjdk.java.net/~dpochepk/8194256/webrev.01/
>
> CR: https://bugs.openjdk.java.net/browse/JDK-8194256
>
>
> I've checked this patch by generating intrinsic which use these
> instructions and verified assembly code.

Great catch.

(I'll note that this mistake would not have been possible with the
integer instructions, which are tested against the GNU assembler's
encodings whenever we run a debug build.)

The resulting logic is complex and very difficult to understand, but I
can see that's not your fault.  Hopefully no-one will ever have to
look at this code again.  OK, thanks.

--
Andrew Haley
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
Reply | Threaded
Open this post in threaded view
|

Re: [aarch64-port-dev ] RFR(S): 8194256 - AARCH64: SIMD shift instructions are incorrectly encoded

Ningsheng Jian
In reply to this post by Dmitrij Pochepko-2
Hi Dmitrij,

I agree that your patch looks better than original logic. But I found
that these instructions are being used in codegen. We need to update
the codegen part with your new logic.

Some jtreg vect cases failed with your patch:

http://openjdk.linaro.org/jdkX/openjdk-jtreg-nightly-tests/reports/server-release/hotspot/aarch64/2018/011/JTreport/html/failed.html

I have created a new bug:

https://bugs.openjdk.java.net/browse/JDK-8195588

Thanks,
Ningsheng

On 28 December 2017 at 22:03, Dmitrij Pochepko
<[hidden email]> wrote:

> Hi all,
>
> please review small patch for 8194256 - AARCH64: SIMD shift instructions are
> incorrectly encoded
>
> I've noticed SIMD shift instructions wrong encoding when trying to use it.
> An intrinsic I was working on, generated incorrect assembly code with wrong
> shift value.
>
> Existing code just copy "shift" bits into instruction bits(immh:immb),
> however, according to spec, it should be encoded as follows:
>
>
> SIMD type <T>:
>
> 8B when immh = 0001 , Q = 0
> 16B when immh = 0001 , Q = 1
> 4H when immh = 001x , Q = 0
> 8H when immh = 001x , Q = 1
> 2S when immh = 01xx , Q = 0
> 4S when immh = 01xx , Q = 1
> 2D when immh = 1xxx , Q = 1
>
>
> <shift> is encoded as follows:
>
>
> for ushr and sshr:
>
> (16-UInt(immh:immb)) when immh = 0001
> (32-UInt(immh:immb)) when immh = 001x
> (64-UInt(immh:immb)) when immh = 01xx
> (128-UInt(immh:immb)) when immh = 1xxx
>
>
> for shl:
>
> (UInt(immh:immb)-8) when immh = 0001
> (UInt(immh:immb)-16) when immh = 001x
> (UInt(immh:immb)-32) when immh = 01xx
> (UInt(immh:immb)-64) when immh = 1xxx
>
>
> So, I've modified respective instruction generation code.
>
>
> webrev: http://cr.openjdk.java.net/~dpochepk/8194256/webrev.01/
>
> CR: https://bugs.openjdk.java.net/browse/JDK-8194256
>
>
> I've checked this patch by generating intrinsic which use these instructions
> and verified assembly code.
>
>
> Thanks,
>
> Dmitrij
>
Reply | Threaded
Open this post in threaded view
|

Re: [aarch64-port-dev ] RFR(S): 8194256 - AARCH64: SIMD shift instructions are incorrectly encoded

Andrew Haley
On 17/01/18 03:21, Ningsheng Jian wrote:

> I agree that your patch looks better than original logic. But I found
> that these instructions are being used in codegen. We need to update
> the codegen part with your new logic.
>
> Some jtreg vect cases failed with your patch:
>
> http://openjdk.linaro.org/jdkX/openjdk-jtreg-nightly-tests/reports/server-release/hotspot/aarch64/2018/011/JTreport/html/failed.html
>
> I have created a new bug:
>
> https://bugs.openjdk.java.net/browse/JDK-8195588

Ouch.  This is bad.  I guess you don't know which were affected?

--
Andrew Haley
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
Reply | Threaded
Open this post in threaded view
|

Re: [aarch64-port-dev ] RFR(S): 8194256 - AARCH64: SIMD shift instructions are incorrectly encoded

Ningsheng Jian
I have created a patch to fix it.

Thanks,
Ningsheng

On 17 January 2018 at 18:08, Andrew Haley <[hidden email]> wrote:

> On 17/01/18 03:21, Ningsheng Jian wrote:
>> I agree that your patch looks better than original logic. But I found
>> that these instructions are being used in codegen. We need to update
>> the codegen part with your new logic.
>>
>> Some jtreg vect cases failed with your patch:
>>
>> http://openjdk.linaro.org/jdkX/openjdk-jtreg-nightly-tests/reports/server-release/hotspot/aarch64/2018/011/JTreport/html/failed.html
>>
>> I have created a new bug:
>>
>> https://bugs.openjdk.java.net/browse/JDK-8195588
>
> Ouch.  This is bad.  I guess you don't know which were affected?
>
> --
> Andrew Haley
> Java Platform Lead Engineer
> Red Hat UK Ltd. <https://www.redhat.com>
> EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671