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

[Vladimir Kozlov]

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
>>