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

[Berg, Michael C]

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.

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