RFR: 8351627: C2 AArch64 ROR/ROL: assert((1 << ((T>>1)+3)) > shift) failed: Invalid Shift value

[Andrew Dinn]

On Fri, 14 Mar 2025 09:43:15 GMT, Xiaohong Gong <xgong at openjdk.org> wrote:

> The following assertion fails on AArch64:
> 
> 
>   Internal Error (jdk/src/hotspot/cpu/aarch64/assembler_aarch64.hpp:2991), pid=3822987, tid=3823007
>   assert((1 << ((T>>1)+3)) > shift) failed: Invalid Shift value
> 
> 
> with a simple Vector API case:
> 
> public static IntVector test() {
>     IntVector iv = IntVector.zero(IntVector.SPECIES_128);
>     return iv.lanewise(VectorOperators.ROR, iv);
> }
> 
> 
> On AArch64, vector `ROR/ROL` (rotate right/left) operations are implemented with a combination of shifts. Please see the pattern for `ROR`:
> 
> 
>   lsr dst1, src, cnt            // unsigned right shift
>   lsl dst2, src, bitSize - cnt  // left shift
>   orr dst, dst1, dst2            // logical or
> 
> where `bitSize` is the element type width (e.g. `32` for `int`). In above case, `cnt` is a zero constant, resulting in a left shift of 32 (`bitSize - 0`), which exceeds the instruction's valid shift count range and triggers the assertion. To fix this, we need to mask the shift count to ensure it stays within valid range when calculating shift counts for rotate operations: `shiftCnt = shiftCnt & (bitSize - 1)`.
> 
> Note that the mask is only necessary for constant shift counts. This not only fixes the assertion failure, but also allows `ROR/ROL src, 0` to be optimized to `src` directly.
> 
> For vector variables as shift counts, the masking can be safely omitted because:
> 1. Vector shift instructions that take a vector register as the shift count may not automatically apply modulo arithmetic based on register size. When the shift count is `32` for int type, the result may be either `zeros` or `src`. However, this doesn't affect correctness for rotate since the final result is combined with `src` using a logical `OR` operation.
> 2. It saves a vector logical `AND` for masking, which is friendly to the performance.

Looks good.

test/hotspot/jtreg/compiler/vectorapi/TestRotateWithZero.java line 80:

> 78: 
> 79:     private static void rotateLeftWithZero() {
> 80:         IntVector vzero = IntVector.zero(I_SPECIES);

This local is unused.

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

Marked as reviewed by adinn (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/24051#pullrequestreview-2689851145
PR Review Comment: https://git.openjdk.org/jdk/pull/24051#discussion_r1998382139