RFR (S): 8151002: Make Assembler methods vextract and vinsert match actual instructions
vladimir.kozlov at oracle.com
Wed Mar 2 23:31:40 UTC 2016
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:
I agree with Vladimir I. that we should have macroassembler instructions
vinserti128high, vinserti128low, etc. instead of passing imm8. It is
Also why we add new nds->is_valid() checks into assembler instructions?
We are going to remove them:
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.
> Incremental from webrev.01:
> On 2016-03-02 13:12, Mikael Vidstedt wrote:
>> Updated webrev:
>> Incremental from webrev.00:
>> Comments below...
>> On 2016-03-01 23:36, Vladimir Ivanov wrote:
>>> Nice cleanup, Mikael!
>>> 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!
>>> ! 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),
>>> + __ 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 :)
>>> 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
>>>> 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
>>>> Special thanks to Mike Berg for helping discuss, co-develop, and test
>>>> the change!
More information about the hotspot-compiler-dev