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

[Berg, Michael C]

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
>