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

Mikael Vidstedt mikael.vidstedt at oracle.com
Mon Mar 7 19:32:17 UTC 2016

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/webr
>>> 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/web
>>>> 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/webrev
>>>>>> /
>>>>>> 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