RFR: 8132160 - support for AVX 512 call frames and stack management

Tue Sep 8 17:34:00 UTC 2015

Ok, the changes are up in:

http://cr.openjdk.java.net/~mcberg/8132160/webrev.03/

We should be ready to proceed.

Thanks
-Michael

-----Original Message-----
From: Berg, Michael C 
Sent: Tuesday, September 08, 2015 9:00 AM
To: 'Vladimir Kozlov'; 'hotspot-compiler-dev at openjdk.java.net'
Subject: RE: RFR: 8132160 - support for AVX 512 call frames and stack management

Yes, that should be done in separate RFE, I like the class method best, as it means only loading one additional parameter field, although in many places.
Then we can curb some of the existing parameters while at it, further tightening the parameter list, making things cleaner still.

I will add the retVal change in .ad file and repost in approx. 1 day.

Thanks,

-Michael

-----Original Message-----
From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com]
Sent: Friday, September 04, 2015 6:20 PM
To: Berg, Michael C; 'hotspot-compiler-dev at openjdk.java.net'
Subject: Re: RFR: 8132160 - support for AVX 512 call frames and stack management

Hi Michael,

This looks better.

One more thing - assembler's attributes (your new fields).

Attributes like next are fine since they are set only once:

_legacy_mode_bw = (VM_Version::supports_avx512bw() == false);

But attributes which could be changed per instruction should not be global:

void Assembler::addsd(XMMRegister dst, Address src) {
   NOT_LP64(assert(VM_Version::supports_sse2(), ""));
   if (VM_Version::supports_evex()) {
     _tuple_type = EVEX_T1S;
     _input_size_in_bits = EVEX_64bit;

They should be either passed as parameters to *_prefix() methods. But it adds more variation of *_prefix() methods.

Or add separate Class and factory methods which return an object (with all attributes set) which you can pass to *_prefix(). This way we may reduce number of parameters passed to *_prefix() methods.
It may be too big change - do it as separate RFE.
I can push current changes and you will work on RFE after that.
What do you think? Other ideas?

Thanks,
Vladimir

On 9/4/15 1:33 PM, Berg, Michael C wrote:
> Vladimir, please find the updated webrev at:
>
> http://cr.openjdk.java.net/~mcberg/8132160/webrev.02/
>
> which address every item below excepting a separate review which will be sent in another email thread for the sqrt vector changes.
> The code was retested for AVX={0,2,3} on 32-bit and 64-bit, with jtreg and SPECjvm2008, using simulators for AVX512 testing.
>
> Thanks,
> Michael
>
> -----Original Message-----
> From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com]
> Sent: Wednesday, September 02, 2015 3:38 PM
> To: Berg, Michael C; 'hotspot-compiler-dev at openjdk.java.net'
> Subject: Re: RFR: 8132160 - support for AVX 512 call frames and stack 
> management
>
> First, split SqrtVD into separate changes.
>
> src/share/vm/utilities/vmError.cpp no needed - the problem is fixed by
> 8133984 already.
>
> assembler_x86.hpp we use _ prefix for fields to distinguish from local
> variables:
>
>    599   int evex_encoding;
>    600   int input_size_in_bits;
>    601   int avx_vector_len;
>    602   int tuple_type;
>    603   bool is_evex_instruction;
>    604   bool legacy_mode_bw;
>    605   bool legacy_mode_dq;
>    606   bool legacy_mode_vl;
>    607   bool legacy_mode_vlbw;
>    608   bool instruction_uses_vl;
>
> otherwise it looks strange:
>
>    void Assembler::movdqa(XMMRegister dst, Address src) {
> +  instruction_uses_vl = true;
>      NOT_LP64(assert(VM_Version::supports_sse2(), ""));
>
> assembler_x86.cpp it is difficult to track what false, true means in next code, for example:
>
> +  emit_simd_arith_nonds(0xE6, dst, src, VEX_SIMD_F3, false, true);
>
> either ass comments ,  /* no_mask_reg*/ false, /* legacy_mode */ true)
>
> or use local vars:
>
> bool no_mask_reg = false;
> bool legacy_mode = true;
>
>
> c1_Runtime1_x86.cpp and other files.
> We usually don't share declaration of loop index (n) if it is not used after loop (sme for 'offset':
>
> for (n = 0; n < FrameMap::nof_fpu_regs; n++) {
>
> I see such pattern in other files too.
>
> It would be nice if you have VM_Version::supports_avx512_xxx() which checks UseAVX:
>
> UseAVX > 2 && VM_Version::supports_avx512vlbw() UseAVX > 2 &&
> (VM_Version::supports_avx512vl() == false)
>
> Use more meaningful names instead of get_xcr0_*():
>
> +    // EDX:EAX is filled in by the intial cpuid state check value
> +    movl(rdx,VM_Version::get_xcr0_edx());
> +    movl(rax,VM_Version::get_xcr0_eax());
>
> Typo: 'intial'. And what is this 'intial cpuid state' anyway?
> How it is related to xrstor/xsave? And xem_xcr0_edx is reserved:
>
> uint32_t     xem_xcr0_edx; // reserved
>
> So what values xem_xcr0_eax and xem_xcr0_edx contains and when it is stored?
>
> I don't see where new bndregs and bndcsr are used.
>
> Thanks,
> Vladimir
>
> On 7/22/15 10:55 PM, Berg, Michael C wrote:
>> Hi Folks,
>>
>> I would like to contribute AVX 512 call frame and stack management 
>> changes. I need two reviewers to examine this patch and comment as needed:
>>
>> Bug-id: https://bugs.openjdk.java.net/browse/JDK-8132160
>>
>> webrev:
>>
>> http://cr.openjdk.java.net/~mcberg/8132160/webrev.01/
>>
>> These changes simplify frame management on 32-bit and 64-bit systems 
>> which support EVEX and extend more complete frame save and restore 
>> functionality as well as stack management for calls, traps and 
>> explicit exception paths. These changes also move CPUID queries into 
>> the assembler object state and add more state rules to a large class 
>> of instructions while simplifying their use. Also added is support 
>> for vectorizing double precision sqrt which is available through the 
>> math library. Many generated stubs and internal functions also now 
>> have predicated mask management for EVEX added.
>>
>> Thanks,
>>
>> Michael
>>