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

Mikael Vidstedt mikael.vidstedt at oracle.com
Wed Mar 2 23:02:43 UTC 2016


After discussing with Vladimir off-list we agreed that changing the type 
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/

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



More information about the hotspot-compiler-dev mailing list