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

[Igor Veresov]

Agree with Vladimir, a more functional style for instruction emission would be better. Depending on the state of the Assembler is a bit fragile. 
Also, a little style nit: in x86.ad, retValue -> ret_value.

igor

> On Sep 4, 2015, at 6:19 PM, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:
> 
> 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
>>>