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

[Vladimir Kozlov]

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