RFR: 8132160 - support for AVX 512 call frames and stack management
Berg, Michael C
michael.c.berg at intel.com
Fri Sep 11 21:55:43 UTC 2015
Vladimir, I have made the updates from below and retested with good results. You can find the resultant webrev at:
ttp://cr.openjdk.java.net/~mcberg/8132160/webrev.04/
Thanks,
Michael
-----Original Message-----
From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com]
Sent: Thursday, September 10, 2015 10:33 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
Big flaw found during JPRT testing - FrameMap is only defined in C1 code:
src/share/vm/c1/c1_FrameMap.hpp:class FrameMap : public CompilationResourceObj {
You can't use it in shared code:
hotspot/src/cpu/x86/vm/sharedRuntime_x86_32.cpp:118:22: error:
'FrameMap' has not been declared
int num_xmm_regs = FrameMap::nof_xmm_regs;
Use:
FloatRegisterImpl::number_of_registers
XMMRegisterImpl::number_of_registers
Thanks,
Vladimir
On 9/8/15 10:34 AM, Berg, Michael C wrote:
> 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
>>>
More information about the hotspot-compiler-dev
mailing list