[jdk18] RFR: 8278267: ARM32: several vector test failures for ASHR [v3]
Dean Long
dlong at openjdk.java.net
Mon Jan 3 22:57:18 UTC 2022
On Tue, 28 Dec 2021 02:32:38 GMT, Hao Sun <haosun at openjdk.org> wrote:
>> In ARM32, "VSHL (register)" instruction [1] is shared by vector left
>> shift and vector right shift, and the condition to distinguish them is
>> whether the shift count value is positve or negative. Hence, negation
>> operation is needed before conducting vector right shift.
>>
>> For vector right shift, the shift count can be a RShiftCntV or a normal
>> vector node. Take test case Byte64VectorTests.java [2][3] as an example.
>> Note that RShiftCntV is already negated via rules "vsrcntD" and
>> "vsrcntX" whereas the normal vector node is NOT, since we don't know
>> whether a normal vector node is used as a vector shift count or not.
>> This is the root cause for these vector test failures.
>>
>> The fix is simple, moving the negation from "vsrcntD|X" to the
>> corresponding vector right shift rules.
>>
>> Affected rules are vsrlBB_reg and vsraBB_reg. Note that vector shift
>> related rules are in form of "vsAABB_CC", where
>> 1) AA can be l (left shift), rl (logical right shift) and ra (arithmetic
>> right shift).
>> 2) BB can be 8B/16B (byte type), 4S/8S (short type), 2I/4I (int type)
>> and 2L (long type).
>> 3) CC can be reg (register case) and immI (immediate case).
>>
>> Minor updates:
>> 1) Merge "vslcntD" and "vsrcntD" into rule "vscntD", as these two rules
>> conduct the same duplication operation now.
>> 2) Update the "match" primitive for vsraBB_immI rules.
>> 3) Style issue: remove the surrounding space for "ins_pipe" primitive.
>>
>> Tests:
>> We ran tier 1~3 tests on ARM32 platform. With this patch, previously
>> failed vector test cases can pass now without introducing test
>> regression.
>>
>> [1] https://developer.arm.com/documentation/ddi0406/c/Application-Level-Architecture/Instruction-Details/Alphabetical-list-of-instructions/VSHL--register-?lang=en
>> [2] https://github.com/openjdk/jdk/blame/master/test/jdk/jdk/incubator/vector/Byte64VectorTests.java#L2237
>> [3] https://github.com/openjdk/jdk/blame/master/test/jdk/jdk/incubator/vector/Byte64VectorTests.java#L2425
>
> Hao Sun has updated the pull request incrementally with one additional commit since the last revision:
>
> Make minimal updates to exisiting rules
>
> 1. logical left shift rules
> a). add is_var_shift check for vslAA_immI rules.
> b). for vslAA_reg rules, remove the matching for URShiftV cases as we
> have the separate logical right shift rules now.
>
> 2. logical right shift rules
> a). add vsrlAA_reg and vsrlAA_reg_var rules.
> b). add is_var_shift check for vsrlAA_immI rules.
>
> 3. arithmetic right shift rules
> a). add is_var_shift check for vsraAA_reg rules.
> b). add vsraAA_reg_var rules
> c). for vsraAA_immI rules, add is_var_shift check and update the match
> primitive.
>
> Code style issues(FIXME and the surrounding space in ins_pipe):
> 1. for modified rules, keep it as it was
> 2. for newly added rules, update the style
Marked as reviewed by dlong (Reviewer).
src/hotspot/cpu/arm/arm.ad line 10795:
> 10793:
> 10794: instruct vsl8B_immI(vecD dst, vecD src, immI shift) %{
> 10795: predicate(n->as_Vector()->length() == 8 && !n->as_ShiftV()->is_var_shift());
Is checking for is_var_shift() really necessary for these immI rules? It seems like it should never happen, so it could be an assert.
src/hotspot/cpu/arm/arm.ad line 10999:
> 10997: "VNEG.S8 $tmp.D,$shift.D\n\t! neg packed8B"
> 10998: "VSHL.U16 $dst.D,$src.D,$tmp.D\t! logical right shift packed4S"
> 10999: %}
This matches what aarch64 is doing, right? I was hoping there was a way for multiple shift instructions to reuse the same negated value, like we do for vsrcntX. But it would probably require introducing a new operand type, like vecNegatedRShiftX, or doing the negate in the IR like we do for the mask.
src/hotspot/share/opto/vectornode.hpp line 546:
> 544: virtual bool cmp(const Node& n) const {
> 545: return VectorNode::cmp(n) && _is_var_shift == ((ShiftVNode&)n)._is_var_shift;
> 546: }
Is it important to add hash() and cmp()?
-------------
PR: https://git.openjdk.java.net/jdk18/pull/41
More information about the hotspot-compiler-dev
mailing list