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

Mon Mar 7 19:32:17 UTC 2016

New webrev:

http://cr.openjdk.java.net/~mikael/webrevs/8151002/webrev.03/webrev/

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

Summary:

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.

Cheers,
Mikael

[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
>>>>>>
>>>>
>>>