RFR(S): 8178811: Minimize the AVX <-> SSE transition penalty on x86

Fri May 5 01:04:48 UTC 2017

Good.
Build passed. Let see how testing will go.

Thanks,
Vladimir

On 5/4/17 5:25 PM, Deshpande, Vivek R wrote:
> HI Vladimir
>
> I have the updated patch at this location:
> http://cr.openjdk.java.net/~vdeshpande/8178811/webrev.02/
> This resolves the problem which comes with FASTDEBUG build.
> Thanks for reviewing and taking a look at it.
>
> Regards,
> Vivek
>
> -----Original Message-----
> From: Deshpande, Vivek R
> Sent: Tuesday, May 02, 2017 12:27 PM
> To: Vladimir Kozlov; hotspot compiler
> Cc: Viswanathan, Sandhya
> Subject: RE: RFR(S): 8178811: Minimize the AVX <-> SSE transition penalty on x86
>
> Hi Vladimir
>
> I will check and get back to you soon.
> Thanks.
>
> Regards,
> Vivek
>
> -----Original Message-----
> From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com]
> Sent: Tuesday, May 2, 2017 11:20 AM
> To: Deshpande, Vivek R; hotspot compiler
> Cc: Viswanathan, Sandhya
> Subject: Re: RFR(S): 8178811: Minimize the AVX <-> SSE transition penalty on x86
>
> The build failed:
>
> #  Internal Error (/scratch/opt/jprt/T/P1/175733.vkozlov/s/hotspot/src/cpu/x86/vm/vm_version_x86.hpp:640), pid=7347, tid=7348 #  assert(_cpuid_info.std_cpuid1_eax.bits.family != 0) failed: VM_Version not initialized
>
> V  [libjvm.so+0x93a6a0]  report_vm_error(char const*, int, char const*, char const*, ...)+0x60 V  [libjvm.so+0x14cb1f0]  VM_Version_StubGenerator::generate_get_cpu_info()+0x2f20
> V  [libjvm.so+0x14c7f53]  VM_Version::initialize()+0x103
>
> I think it fails at check is_intel() in set_avx_cpuFeatures() or set_evex_cpuFeatures().
> Please, build fastdebug JVM for testing.
>
> Vladimir
>
> On 5/2/17 10:59 AM, Vladimir Kozlov wrote:
>> Looks good. One nit - move next code into separate function instead of duplicating 3 times:
>>
>> +    if( is_intel() ) { // Intel cpus specific settings
>> +      if ((cpu_family() == 0x06) &&
>> +          ((extended_cpu_model() == 0x57) ||   // Xeon Phi 3200/5200/7200
>> +          (extended_cpu_model() == 0x85))) {  // Future Xeon Phi
>> +        _features &= ~CPU_VZEROUPPER;
>> +      }
>> +    }
>>
>> I will run testing for current changes and let you know results.
>>
>> Thanks,
>> Vladimir
>>
>> On 4/20/17 1:54 PM, Deshpande, Vivek R wrote:
>>> HI Vladimir
>>>
>>> We added almost all the vzeroupper instructions by analyzing SPECjbb2015.
>>>
>>> We need vzeroupper after we execute avx2/avx-512 and before
>>> transition to SSE to avoid penalty of saving and restoring higher bits in YMM/ZMM registers. Since JVM is SSE compiled, mixing of AVX and SSE code happens in C1 and interpreter with usage of intrinsics, so I have added vzerouppers at these transitions in the stubs.
>>>
>>> I think max_vector_size() > 16 would be with auto vectorization, but we need to have vzeroupper with stubs and intrinsics.
>>>
>>> I have made all the changes which you suggested. Please take a look at the patch and let me know your thoughts.
>>>
>>> The updated Webrev is here:
>>> http://cr.openjdk.java.net/~vdeshpande/8178811/webrev.01/
>>>
>>> Thank you.
>>> Regards,
>>> Vivek
>>>
>>> -----Original Message-----
>>> From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com]
>>> Sent: Friday, April 14, 2017 12:48 PM
>>> To: Deshpande, Vivek R; hotspot compiler
>>> Cc: Viswanathan, Sandhya
>>> Subject: Re: RFR(S): 8178811: Minimize the AVX <-> SSE transition
>>> penalty on x86
>>>
>>> Hi Vivek,
>>>
>>> Did you pinpoint particular change which helps SPECjbb2015?
>>>
>>>  From what I remember it is mostly during call to runtime or JNI calls.
>>>
>>>    // AVX instruction which is used to clear upper 128 bits of YMM registers and
>>>    // to avoid transaction penalty between AVX and SSE states. There is no
>>>    // penalty if legacy SSE instructions are encoded using VEX prefix because
>>>    // they always clear upper 128 bits. It should be used before calling
>>>    // runtime code and native libraries.
>>>    void vzeroupper();
>>>
>>> So why you added vzeroupper() into arraycopy and other stubs? If AVX is supported all instructions should be encoded with VEX prefix. Or the statement in the comment is incorrect?
>>>
>>> About UseVzeroupper flag. We should avoid adding new product flags if possible.
>>> IT only make sense if you want to do experiments to check performance with and without it. But we already know that it is needed on most CPUs.
>>>
>>> I would suggest to add VM_Version::supports_vzeroupper() { return
>>> (_features & CPU_VZEROUPPER) != 0; } Set CPU_VZEROUPPE if AVX is supported and clear it for Knights CPU. The add check inside assembler instruction:
>>>
>>>    void Assembler::vzeroupper() {
>>> -   assert(VM_Version::supports_avx(), "");
>>> +   if (VM_Version::supports_vzeroupper()) {
>>>
>>> It will allow to avoid supports_avx() checks on each vzeroupper() call.
>>>
>>> You missed check in MachEpilogNode::emit() in x86_64.ad
>>>
>>> Note, I used (C->max_vector_size() > 16) checks in .ad files because of the same reasons as above:
>>>
>>>    // There is no penalty if legacy SSE instructions are encoded using VEX prefix because
>>>    // they always clear upper 128 bits.
>>>
>>> I thought if vectors are small and we use VEX prefix then upper bits will be 0 anyway so you don't need vzeroupper().
>>>
>>> Thanks,
>>> Vladimir
>>>
>>> On 4/14/17 11:17 AM, Deshpande, Vivek R wrote:
>>>> Hi All
>>>>
>>>>
>>>>
>>>> This fix minimizes the AVX to SSE and SSE to AVX transition penalty
>>>> through generation of vzeroupper instruction. With this patch we see
>>>> zero transitions with penalty per SPECjbb2015 jOPS on BDW and a
>>>> significant reduction on SKX CPU event vector width mismatch from 65
>>>> to 0.01 per SPECjbb2015 jOPS. We have also implemented an enhancement to disable vzeroupper generation for Knights family where the instruction has high penalty and is not recommended. The option UseVzeroupper is used to control generation of vzeroupper instruction and gets set to false on the Knights family.
>>>> We observed ~3% gain on SPECJvm2008 composite result on Skylake.
>>>>
>>>> Webrev:
>>>>
>>>> http://cr.openjdk.java.net/~vdeshpande/8178811/webrev.00/
>>>>
>>>> I have also updated the JBS entry.
>>>>
>>>> https://bugs.openjdk.java.net/browse/JDK-8178811
>>>>
>>>> Would you please review and sponsor it.
>>>>
>>>>
>>>>
>>>> Regards,
>>>>
>>>> Vivek
>>>>
>>>>
>>>>