RFR: 8332119: Incorrect IllegalArgumentException for C2 compiled permute kernel [v2]

[Vladimir Kozlov]

On Sun, 2 Jun 2024 15:43:39 GMT, Jatin Bhateja <jbhateja at openjdk.org> wrote:

>> Currently inline expansion of vector to shuffle conversion simply type casts the vector holding indexes to byte vector[1] where as fallback implementation[2] also wraps the indexes to a valid index range [0, VEC_LEN-1) or generates a -ve index for exceptional / OOB indices.
>> 
>> This patch extends the conversion inline expander to match the fall back implementation. This imposes around 20% performance tax on Vector.toShuffle() intrinsic but fixes this functional bug.
>> 
>> Kindly review and share your feedback.
>> 
>> Best Regards,
>> Jatin
>> 
>> PS: Patch also fixes an incorrectness issue reported with [JDK-8332118](https://bugs.openjdk.org/browse/JDK-8332118)
>> 
>> [1] https://github.com/openjdk/jdk/blob/master/src/jdk.incubator.vector/share/classes/jdk/incubator/vector/FloatVector.java#L2352
>> [2] https://github.com/openjdk/jdk/blob/master/src/jdk.incubator.vector/share/classes/jdk/incubator/vector/AbstractShuffle.java#L58
>
> Jatin Bhateja has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Review Comments Incorporated.

I have some comments.

src/hotspot/share/opto/vectorIntrinsics.cpp line 514:

> 512: }
> 513: 
> 514: Node* LibraryCallKit::partially_wrap_indexes(Node* index_vec, int num_elem, BasicType elem_bt) {

Can you add comment with pseudo code to show what this method do?

src/hotspot/share/opto/vectorIntrinsics.cpp line 517:

> 515:   assert(elem_bt == T_BYTE, "");
> 516:   const TypeVect * vt  = TypeVect::make(elem_bt, num_elem);
> 517:   const Type * type_bt = Type::get_const_basic_type(elem_bt);

Please remove space between a type and `*`.

src/hotspot/share/opto/vectorIntrinsics.cpp line 622:

> 620:      res = gvn().transform(VectorNode::make(Op_AndV, res, bcast_mod, vt));
> 621:   } else {
> 622:      res = partially_wrap_indexes(res, num_elem, elem_bt);

Original spacing here was correct. One at the line 621 is wrong and have to be fixed.

src/hotspot/share/opto/vectorIntrinsics.cpp line 2308:

> 2306:        !arch_supports_vector(Op_AndV, num_elem_to, elem_bt_to, VecMaskNotUsed)            ||
> 2307:        !arch_supports_vector(Op_Replicate, num_elem_to, elem_bt_to, VecMaskNotUsed))) {
> 2308:     return false;

Please add `log_if_needed("` here too.

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

PR Review: https://git.openjdk.org/jdk/pull/19442#pullrequestreview-2097045286
PR Review Comment: https://git.openjdk.org/jdk/pull/19442#discussion_r1626403018
PR Review Comment: https://git.openjdk.org/jdk/pull/19442#discussion_r1626398694
PR Review Comment: https://git.openjdk.org/jdk/pull/19442#discussion_r1626407248
PR Review Comment: https://git.openjdk.org/jdk/pull/19442#discussion_r1626406018