RFR(M): 8213134: AArch64: vector shift failed with MaxVectorSize=8

[Vladimir Kozlov]

Looks good.

What testing you did?
Unfortunately compiler/c2/cr634086 test is not part of tier1 testing and not executed by 'submit' repo testing. You need 
to run it.

Thanks,
Vladimir

On 11/28/18 12:32 AM, Yang Zhang (Arm Technology China) wrote:
> Hi Andrew, Vladimir
> 
> I update the reviewers and remove the new assert in Matcher::vector_shift_count_ideal_reg(). Could you please help review again?
> 
> http://cr.openjdk.java.net/~yzhang/8213134/webrev.02/
> 
> Regards
> Yang
> 
> -----Original Message-----
> From: Vladimir Kozlov <vladimir.kozlov at oracle.com>
> Sent: Thursday, November 22, 2018 1:56 AM
> To: Yang Zhang (Arm Technology China) <Yang.Zhang at arm.com>; hotspot-dev at openjdk.java.net
> Subject: Re: RFR(M): 8213134: AArch64: vector shift failed with MaxVectorSize=8
> 
> You are right. I think I was confused that you are not limiting MaxVectorSize but simple use min(16, MaxVectorSize).
> 
> The assert is fine.
> 
> Thanks,
> Vladimir
> 
> On 11/20/18 10:28 PM, Yang Zhang (Arm Technology China) wrote:
>>
>>> I don't see any checking or setting for MaxVectorSize in vm_version_aarch64.cpp.
>>> In such case you will definitely hit your new assert Matcher::vector_shift_count_ideal_reg().
>>> aarch64.ad code Matcher::vector_width_in_bytes() has limit vector size to 16 bytes. So using  MaxVectorSize > 16 will hit the assert.
>>
>> Matcher::vector_width_in_bytes() limits vector size to min(16, MaxVectorSize).
>> Matcher::vector_shift_count_ideal_reg() assert(MaxVectorSize >= size,
>> "Length isn't supported"); When MaxVectorSize is 8, size is 8.
>> When MaxVectorSize is greater than or equal 16, size is 16.
>> So (MaxVectorSize >= size) is always true.
>>
>> Test cases of cr6340864 are passed with or without this new  assert.
>>
>> Do I need to remove this new assert?
>>
>> Regards
>> Yang
>>
>>
>> On 11/18/18 11:53 PM, Yang Zhang (Arm Technology China) wrote:
>>> Hi,
>>>
>>> When I implemented AArch64 NEON for Vector API (http://openjdk.java.net/jeps/338), I found an issue about vector shift. I have a patch which could fix this issue. Could anyone help to review this patch?
>>>
>>> Webrev: http://cr.openjdk.java.net/~yzhang/8213134/webrev.01/
>>> JBS:  https://bugs.openjdk.java.net/browse/JDK-8213134
>>>
>>> This patch is verified both in jdk/jdk master and panama/vectorIntrinsics, and tests are passed.
>>> Hotspt jtreg contains the following test cases that can cover all the patterns of vector shift.  But only default option "-XX:MaxVectorSize=64" is tested, so this issue isn't detected. Now I add "-XX:MaxVectorSize=8", "-XX:MaxVectorSize=16" and "-XX:MaxVectorSize=32" to these jtreg tests in this patch.
>>> compiler/c2/cr6340864/TestByteVect.java
>>> compiler/c2/cr6340864/TestIntVect.java
>>> compiler/c2/cr6340864/TestShortVect.java
>>> compiler/codegen/TestCharVect2.java
>>>
>>> Regards
>>> Yang
>>>
>>>
>> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
>>
> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
>