[jdk18] RFR: 8278267: ARM32: several vector test failures for ASHR

[Hao Sun]

On Tue, 21 Dec 2021 09:53:07 GMT, Dean Long <dlong 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
>
> There seems to be an interesting history here.
> 
> Method 1: negate in RShiftCntV rule
> Method 2: negate in RShiftV* rules
> 
> For aarch64, the negate was moved into the shift instruction in JDK-8213134 (Method 1 --> Method 2).  Then JDK-8262916 proposed to move it back out of the shift instruction again.  In that PR, the opinion that Method 1 (arm32) generated better code than Method 2 (aarch64) was expressed.  Now it looks like this PR proposes for arm32 to move to Method 2 like aarch64, so I suspect that there will be a performance impact.
> 
> I think there is a simpler fix that doesn't require moving where the negate happens.  JDK-8277239 fixed a similar problem by adding a flag on vector shift nodes to indicate variable shift, then checking the flag in the predicate.  Perhaps arm32 could do the same?

Thanks for your reviews! @dean-long @nsjian 
I suppose this patch is ready to be integrated.

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

PR: https://git.openjdk.java.net/jdk18/pull/41