RFR(XS):8216580:X86: Fix generation of VNNI vector code by allowing adjacent LoadS nodes to be isomorphic

Tue Mar 5 22:09:00 UTC 2019

Thanks Vladimir.
I have pushed the change.

Regards,
Vivek

-----Original Message-----
From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com] 
Sent: Tuesday, March 5, 2019 12:40 PM
To: Deshpande, Vivek R <vivek.r.deshpande at intel.com>; 'Tobias Hartmann' <tobias.hartmann at oracle.com>; 'hotspot-compiler-dev at openjdk.java.net compiler' <hotspot-compiler-dev at openjdk.java.net>
Cc: Viswanathan, Sandhya <sandhya.viswanathan at intel.com>; Raj, Guru <guru.raj at intel.com>
Subject: Re: RFR(XS):8216580:X86: Fix generation of VNNI vector code by allowing adjacent LoadS nodes to be isomorphic

Looks good.

Thanks,
Vladimir

On 3/4/19 4:34 PM, Deshpande, Vivek R wrote:
> Hi Vladimir
> 
> I have tested the patch with compiler tests on VNNI h/w and it passed.
> While doing tests in jdk, I noticed that the checks should be guarded against NULL.
> So I have added those checks:
> if(s1_ctrl != NULL && s2_ctrl != NULL) { ...
> The webrev is here:
> http://cr.openjdk.java.net/~vdeshpande/8216580/webrev.03/
> I have also rebased the patch on jdk/jdk.
> 
> Regards,
> Vivek
> 
> -----Original Message-----
> From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com]
> Sent: Friday, March 1, 2019 1:47 PM
> To: Deshpande, Vivek R <vivek.r.deshpande at intel.com>; 'Tobias 
> Hartmann' <tobias.hartmann at oracle.com>; 
> 'hotspot-compiler-dev at openjdk.java.net compiler' 
> <hotspot-compiler-dev at openjdk.java.net>
> Cc: Viswanathan, Sandhya <sandhya.viswanathan at intel.com>; Raj, Guru 
> <guru.raj at intel.com>
> Subject: Re: RFR(XS):8216580:X86: Fix generation of VNNI vector code 
> by allowing adjacent LoadS nodes to be isomorphic
> 
> My testing passed. I think you can push after you finish testing.
> Please, re-base you changes to jdk/jdk repository before push. I see that webrev.02 was prepared vs jdk/jdk12 which is wrong.
> 
> Thanks,
> Vladimir
> 
> On 3/1/19 11:53 AM, Deshpande, Vivek R wrote:
>> Hi Vladimir
>>
>> Thanks for the review. I am also working on testing it on the VNNI enabled h/w.
>>
>> Regards,
>> Vivek
>>
>> -----Original Message-----
>> From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com]
>> Sent: Friday, March 1, 2019 10:02 AM
>> To: Deshpande, Vivek R <vivek.r.deshpande at intel.com>; 'Tobias 
>> Hartmann' <tobias.hartmann at oracle.com>; 
>> 'hotspot-compiler-dev at openjdk.java.net compiler'
>> <hotspot-compiler-dev at openjdk.java.net>
>> Cc: Viswanathan, Sandhya <sandhya.viswanathan at intel.com>; Raj, Guru 
>> <guru.raj at intel.com>
>> Subject: Re: RFR(XS):8216580:X86: Fix generation of VNNI vector code 
>> by allowing adjacent LoadS nodes to be isomorphic
>>
>> This looks good. I assume you did full testing of these new changes on VNNI machine. I will submit testing on what we have.
>>
>> Thanks,
>> Vladimir
>>
>> On 2/28/19 5:23 PM, Deshpande, Vivek R wrote:
>>> Hi Vladimir
>>>
>>> Thanks for your inputs. I have made the changes according to your suggestion.
>>> The webrev is here:
>>> http://cr.openjdk.java.net/~vdeshpande/8216580/webrev.02/
>>> This addresses the questions you had raised.
>>> With this patch the checks are applied to all the nodes but returns true only in case of muladds2i.
>>>
>>> Regards,
>>> Vivek
>>>
>>> -----Original Message-----
>>> From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com]
>>> Sent: Wednesday, February 13, 2019 12:29 PM
>>> To: Deshpande, Vivek R <vivek.r.deshpande at intel.com>; 'Tobias 
>>> Hartmann' <tobias.hartmann at oracle.com>; 
>>> 'hotspot-compiler-dev at openjdk.java.net compiler'
>>> <hotspot-compiler-dev at openjdk.java.net>
>>> Cc: Viswanathan, Sandhya <sandhya.viswanathan at intel.com>; Raj, Guru 
>>> <guru.raj at intel.com>
>>> Subject: Re: RFR(XS):8216580:X86: Fix generation of VNNI vector code 
>>> by allowing adjacent LoadS nodes to be isomorphic
>>>
>>> Hi Vivek,
>>>
>>> Most of new checks are loop invariant: !s1_ctrl_inv and
>>> !s1_ctrl->is_RangeCheck()
>>>
>>> I think you don't need to search for is_muladds2i() if those checks return false.
>>>
>>> Most general question is: why it should apply only to muladds2i nodes only? Can we do the same for others?
>>>
>>> Thanks,
>>> Vladimir
>>>
>>> On 2/8/19 2:17 PM, Deshpande, Vivek R wrote:
>>>> Hi Vladimir
>>>>
>>>> Would you please take a look at this patch.
>>>>
>>>> The Adjacent LoadS have different control RangeCheck node for accesses of type a[2i] and a[2i+1].
>>>> This patch allows those nodes to be isomorphic as they belong same counted loop and MulAddS2I nodes.
>>>>      
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~vdeshpande/8216580/webrev.01/
>>>> Bug ID:
>>>> https://bugs.openjdk.java.net/browse/JDK-8216580
>>>>
>>>> Regards,
>>>> Vivek
>>>>
>>>> -----Original Message-----
>>>> From: Deshpande, Vivek R
>>>> Sent: Monday, January 28, 2019 9:45 AM
>>>> To: Tobias Hartmann <tobias.hartmann at oracle.com>; 
>>>> hotspot-compiler-dev at openjdk.java.net compiler 
>>>> <hotspot-compiler-dev at openjdk.java.net>; Vladimir Kozlov 
>>>> <vladimir.kozlov at oracle.com>
>>>> Cc: Viswanathan, Sandhya <sandhya.viswanathan at intel.com>; Raj, Guru 
>>>> <guru.raj at intel.com>
>>>> Subject: RE: RFR(XS):8216580:X86: Fix generation of VNNI vector 
>>>> code by allowing adjacent LoadS nodes to be isomorphic
>>>>
>>>> Hi Vladimir
>>>>
>>>> Would you please take a look at the patch.
>>>> The Adjacent LoadS have different control RangeCheck node for accesses of type a[2i] and a[2i+1].
>>>> This patch allows those nodes to be isomorphic as they belong same counted loop and MulAddS2I nodes.
>>>>      
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~vdeshpande/8216580/webrev.01/
>>>>
>>>> Regards,
>>>> Vivek
>>>>
>>>> -----Original Message-----
>>>> From: Tobias Hartmann [mailto:tobias.hartmann at oracle.com]
>>>> Sent: Tuesday, January 15, 2019 2:57 AM
>>>> To: Deshpande, Vivek R <vivek.r.deshpande at intel.com>; 
>>>> hotspot-compiler-dev at openjdk.java.net compiler 
>>>> <hotspot-compiler-dev at openjdk.java.net>
>>>> Cc: Vladimir Kozlov <vladimir.kozlov at oracle.com>; Viswanathan, 
>>>> Sandhya <sandhya.viswanathan at intel.com>; Raj, Guru 
>>>> <guru.raj at intel.com>
>>>> Subject: Re: RFR(XS):8216580:X86: Fix generation of VNNI vector 
>>>> code by allowing adjacent LoadS nodes to be isomorphic
>>>>
>>>> Hi Vivek,
>>>>
>>>> please add parentheses around the == comparison in lines 1225,1226.
>>>>
>>>> Otherwise this looks reasonable to me but I'm not too familiar with that code.
>>>>
>>>> Best regards,
>>>> Tobias
>>>>
>>>> On 12.01.19 01:03, Deshpande, Vivek R wrote:
>>>>> Hi Tobias
>>>>>
>>>>> The webrev for the bug JDK-821650 is here:
>>>>> http://cr.openjdk.java.net/~vdeshpande/8216580/webrev.00/
>>>>> This fixes generation of vector code by allowing adjacent LoadS nodes to be isomorphic when they have different control RangeCheck nodes for a[i] and a[i+1] accesses in same MulAddS2I node.
>>>>> Could you please review it.
>>>>>
>>>>> Regards,
>>>>> Vivek
>>>>>
>>>>> -----Original Message-----
>>>>> From: Deshpande, Vivek R
>>>>> Sent: Friday, January 11, 2019 11:38 AM
>>>>> To: 'Tobias Hartmann' <tobias.hartmann at oracle.com>; 
>>>>> hotspot-compiler-dev at openjdk.java.net compiler 
>>>>> <hotspot-compiler-dev at openjdk.java.net>
>>>>> Cc: Vladimir Kozlov <vladimir.kozlov at oracle.com>; Viswanathan, 
>>>>> Sandhya <sandhya.viswanathan at intel.com>; Raj, Guru 
>>>>> <guru.raj at intel.com>
>>>>> Subject: RE: RFR(S):8216050:X86: Fix for Superword optimization 
>>>>> fails with assert(0 <= i && i < _len) failed: illegal index
>>>>>
>>>>> Hi Tobias
>>>>>
>>>>> Thanks for reviewing the patch.
>>>>> I have made the changes according to your suggestion.
>>>>> In this webrev:
>>>>> http://cr.openjdk.java.net/~vdeshpande/8216050/webrev.01/
>>>>> I have fix for the crash reported in the 8216050.
>>>>>
>>>>> The lower cost is needed for generation of vpdpwssd instruction, by combining AddVI and MulAddVS2VI.
>>>>> For other instructions pmaddwd and vpmaddwd, they get generated on platforms upto skylake with default cost.
>>>>>
>>>>> I have updated the bug also with the link to webrev.
>>>>>
>>>>> I have created a different bug JDK-8216580 for
>>>>>      3) Fix generation of vector code by allowing adjacent LoadS nodes to be isomorphic when they have different control RangeCheck nodes
>>>>>          for a[i] and a[i+1] accesses in same MulAddS2I node
>>>>>
>>>>> Thank you.
>>>>> Regards,
>>>>> Vivek
>>>>>
>>>>> -----Original Message-----
>>>>> From: Tobias Hartmann [mailto:tobias.hartmann at oracle.com]
>>>>> Sent: Friday, January 11, 2019 4:49 AM
>>>>> To: Deshpande, Vivek R <vivek.r.deshpande at intel.com>; 
>>>>> hotspot-compiler-dev at openjdk.java.net compiler 
>>>>> <hotspot-compiler-dev at openjdk.java.net>
>>>>> Cc: Vladimir Kozlov <vladimir.kozlov at oracle.com>; Viswanathan, 
>>>>> Sandhya <sandhya.viswanathan at intel.com>; Raj, Guru 
>>>>> <guru.raj at intel.com>
>>>>> Subject: Re: RFR(S):8216050:X86: Fix for Superword optimization 
>>>>> fails with assert(0 <= i && i < _len) failed: illegal index
>>>>>
>>>>> Hi Vivek,
>>>>>
>>>>> On 11.01.19 07:58, Deshpande, Vivek R wrote:
>>>>>> 1) Fix for the crash by matching the operand by swapping to right positions.
>>>>>
>>>>> Looks good but the change to loopopts.cpp:530 screwed up the indentation around the ifs, please fix.
>>>>>
>>>>>> 2) Cost based generation of vpdpwssd instruction.
>>>>>
>>>>> Other instructions added by JDK-8214751 still miss a cost definition, for example:
>>>>> http://hg.openjdk.java.net/jdk/jdk/rev/4bb6e0871bf7#l5.20
>>>>>
>>>>>> 3) Fix generation of vector code by allowing adjacent LoadS nodes 
>>>>>> to be isomorphic when they have different control RangeCheck nodes
>>>>>>         for a[i] and a[i+1] accesses in same MulAddS2I node
>>>>>
>>>>> This is unrelated to the original bug, right? If so, this should be integrated with a separate RFE.
>>>>>
>>>>> Thanks,
>>>>> Tobias
>>>>>