RFR (S): 8151002: Make Assembler methods vextract and vinsert match actual instructions

Berg, Michael C michael.c.berg at intel.com
Mon Mar 7 21:01:32 UTC 2016

Looks ok to me.


-----Original Message-----
From: Mikael Vidstedt [mailto:mikael.vidstedt at oracle.com] 
Sent: Monday, March 07, 2016 11:32 AM
To: Vladimir Ivanov; Berg, Michael C; hotspot-compiler-dev at openjdk.java.net
Subject: Re: RFR (S): 8151002: Make Assembler methods vextract and vinsert match actual instructions

New webrev:


Unfortunately I think I messed up the incremental webrev, so in order to not cause confusion I'm not including it here, sorry.


I added pseudo instructions for inserting/extracting _low and _high parts of the vector registers. Note that it only applies for the cases where there is a clear low and high to speak of - that is, in the cases where the instruction operates on the high or low *half* of a register. 
For instructions like vinsert32x4 (128bit copy to/from a 512 bit
register) there are four different sources/targets, so high and low are not applicable, so there are no pseudo instructions for them.

The macroAssembler methods now take a uint8_t as well, this was accidentally left out in the last webrev.

I kept the nds->is_valid() checks for now, cleaning that up is covered by JDK-8151003[1].

I also updated a couple of comments.


[1] https://bugs.openjdk.java.net/browse/JDK-8151003

On 3/3/2016 1:16 PM, Vladimir Ivanov wrote:
> On 3/3/16 7:08 AM, Berg, Michael C wrote:
>> Vladimir (K), just for the time being as the problem isn't just 
>> confined to these instructions (the nds issue). I have assigned the 
>> bug below to myself and will take a holistic view over the issue in 
>> its full context.
>> The instructions modified in the webrev, like in the documentation 
>> that exists regarding their definitions, are all programmable via 
>> what is loosely labeled as the imm8 field in the formal 
>> documentation.  I think we should leave them that way.  The onus of 
>> these changes was to make instructions look more like their ISA 
>> manual definitions.  I think Vladimir Ivanov was saying, and please 
>> chime in Vladimir if I do not interpret correctly, wasn't high/low, 
>> it was leaving a signature like what we had in place in the macro 
>> assembler, and invoking the precise names there.  I don't think that 
>> is needed though, as the macro assembler's job is to interpret a 
>> meaning and do a mapping.
> I'm all for the proposed change in Assembler.
> My point is that vmovdqu/vinserti128h/vextracti128h(...) are more 
> informative than vinserti128(...,0/1) & vextracti128(..., 0/1) in the 
> code. So, keeping original functions in the MacroAssembler, but 
> migrating them to new Assembler versions looks reasonable.
> But I can live with both variaints.
> Best regards,
> Vladimir Ivanov
>> Regards,
>> Michael
>> -----Original Message-----
>> From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com]
>> Sent: Wednesday, March 02, 2016 3:32 PM
>> To: hotspot-compiler-dev at openjdk.java.net
>> Cc: Berg, Michael C
>> Subject: Re: RFR (S): 8151002: Make Assembler methods vextract and 
>> vinsert match actual instructions
>> On 3/2/16 3:02 PM, Mikael Vidstedt wrote:
>>> After discussing with Vladimir off-list we agreed that changing the 
>>> type
>> It was Vladimir Ivanov.
>>> of the immediate (imm8) argument to uint8_t is both clearer, has the 
>>> potential to catch incorrect uses of the functions, and also makes 
>>> the asserts more straightforward. In addition to that Vladimir noted 
>>> that I had accidentally included newline in the assert messages.
>>> New webrev:
>>> Full:
>>> http://cr.openjdk.java.net/~mikael/webrevs/8151002/webrev.02/webrev/
>> I agree with Vladimir I. that we should have macroassembler 
>> instructions vinserti128high, vinserti128low, etc. instead of passing 
>> imm8. It is more informative.
>> Also why we add new nds->is_valid() checks into assembler instructions?
>> We are going to remove them:
>> https://bugs.openjdk.java.net/browse/JDK-8151003
>> I know that Mikael had a discussion about this with Michael. So I 
>> would like to see arguments here. Michael?
>> Current code always pass correct registers and x86 Manual requires to 
>> have valid registers.
>> Thanks,
>> Vladimir
>>> Incremental from webrev.01:
>>> http://cr.openjdk.java.net/~mikael/webrevs/8151002/webrev.02.incr/we
>>> br
>>> ev/
>>> Cheers,
>>> Mikael
>>> On 2016-03-02 13:12, Mikael Vidstedt wrote:
>>>> Updated webrev:
>>>> http://cr.openjdk.java.net/~mikael/webrevs/8151002/webrev.01/webrev
>>>> /
>>>> Incremental from webrev.00:
>>>> http://cr.openjdk.java.net/~mikael/webrevs/8151002/webrev.01.incr/w
>>>> eb
>>>> rev/
>>>> Comments below...
>>>> On 2016-03-01 23:36, Vladimir Ivanov wrote:
>>>>> Nice cleanup, Mikael!
>>>>> src/cpu/x86/vm/assembler_x86.hpp:
>>>>> Outdated comments:
>>>>>      // Copy low 128bit into high 128bit of YMM registers.
>>>>>      // Load/store high 128bit of YMM registers which does not 
>>>>> destroy other half.
>>>>>     // Copy low 256bit into high 256bit of ZMM registers.
>>>> Updated, thanks for catching!
>>>>> src/cpu/x86/vm/assembler_x86.cpp:
>>>>> !   emit_int8(imm8 & 0x01);
>>>>> Maybe additionally assert valid imm8 range?
>>>> Good idea, I had added asserts earlier but removed them. I added 
>>>> them back again!
>>>>> Maybe keep vinsert*h variants and move them to MacroAssembler? 
>>>>> They look clearer in some contextes:
>>>>> -      __ vextractf128h(Address(rsp, base_addr+n*16),
>>>>> as_XMMRegister(n));
>>>>> +      __ vextractf128(Address(rsp, base_addr+n*16),
>>>>> as_XMMRegister(n), 1);
>>>> Can I suggest that we try to live without them for a while and see 
>>>> how much we miss them? I think having it there may actually be more 
>>>> confusing in many cases :)
>>>> Cheers,
>>>> Mikael
>>>>> Otherwise, looks good.
>>>>> Best regards,
>>>>> Vladimir Ivanov
>>>>> On 3/2/16 3:25 AM, Mikael Vidstedt wrote:
>>>>>> Please review the following change which updates the various
>>>>>> vextract* and vinsert* methods in assembler_x86 &
>>>>>> macroAssembler_x86 to better match the real HW instructions, 
>>>>>> which also has the benefit of providing the full 
>>>>>> functionality/flexibility of the instructions where earlier only 
>>>>>> some specific modes were supported. Put differently, with this 
>>>>>> change it's much easier to correlate the methods to the Intel 
>>>>>> manual and understand what they actually do.
>>>>>> Specifically, the vinsert* family of instructions take three 
>>>>>> registers and an immediate which decide how the bits should be 
>>>>>> shuffled around, but without this change the method only allowed 
>>>>>> two of the registers to be specified, and the immediate was 
>>>>>> hard-coded to 0x01.
>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8151002
>>>>>> Webrev:
>>>>>> http://cr.openjdk.java.net/~mikael/webrevs/8151002/webrev.00/webr
>>>>>> ev
>>>>>> /
>>>>>> Special thanks to Mike Berg for helping discuss, co-develop, and 
>>>>>> test the change!
>>>>>> Cheers,
>>>>>> Mikael

More information about the hotspot-compiler-dev mailing list