RFR: 8257137: Revise smov and umov in aarch64 assembler [v4]

[Andrew Haley]

On Thu, 25 Feb 2021 01:27:52 GMT, Hao Sun <github.com+16932759+shqking at openjdk.org> wrote:

>> 1. Both smov and umov lack of checking the register type validity.
>>    Register type must be 'B', 'H' or 'S' for smov [1].
>>    Register type can NOT be 'Q' for umov [2].
>>    Such checks are added.
>> 
>> 2. smov and umov have different explanations on 'Q' field, i.e. bit 30
>> of the insturction, but current assembler implementation mixed it up.
>>    For umov, 'Q' field can only be set when register type 'D' is given
>>    [2]. However, this field of smov must be set for register type 'S'
>>    [1], that is, 'Q' field can be optional for register type 'B' or 'H'.
>> 
>>    Current implementation only took the umov scenario into account. As a
>>    result, runtime error ILL_ILLOPN would occur if 'smov(Register,
>> 		   FloatRegister, S, index)' is used.
>> 
>>    We put them into two separate functions and make 'Q' field always set
>>    for smov. That means 'SMOVX' (64-bit register variant) is generated
>>    for all cases since it's compatible with our current usages of 'SMOVW'.
>>    Existing usages of smov have been double checked and this patch does
>>    not affect them.
>> 
>> 3. Smoke tests are also added.
>> 
>> [1]. https://developer.arm.com/docs/ddi0602/f/simd-and-floating-point-instructions-alphabetic-order/smov-signed-move-vector-element-to-general-purpose-register
>> [2]. https://developer.arm.com/docs/ddi0602/f/simd-and-floating-point-instructions-alphabetic-order/umov-unsigned-move-vector-element-to-general-purpose-register
>> 
>> 
>> Note that Jtreg tier1 and jdk::tier3 have been tested and all tests passed without new failures.
>
> Hao Sun has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Code style: add spaces between operands
>   
>   add spaces between operands

src/hotspot/cpu/aarch64/assembler_aarch64.hpp line 2664:

> 2662:     assert(T != Q, "invalid register variant");
> 2663:     f(0, 31), f(T == D ? 1 : 0, 30), f(0b001110000, 29, 21);
> 2664:     f(((idx << 1) | 1) << (int)T, 20, 16), f(0b001111, 15, 10);

(T == D ? 1 : 0)``` is just ```(T == D)```, isn't it?

src/hotspot/cpu/aarch64/assembler_aarch64.hpp line 2675:

> 2673:     rf(Vn, 5), rf(Rd, 0);
> 2674:   }
> 2675: 

I don't think the code duplication here is necessary. Please pass the validity test and the expression for Bit 30 into ```INSN```. It takes a few minutes studying the difference between those two in order to discover what the differences between them are, which could be saved if we made it explicit.

-------------

PR: https://git.openjdk.java.net/jdk/pull/1586