RFR: 8340079: Modify rearrange/selectFrom Vector API methods to perform wrapIndexes instead of checkIndexes [v3]

[Sandhya Viswanathan]

On Thu, 19 Sep 2024 07:29:11 GMT, Jatin Bhateja <jbhateja at openjdk.org> wrote:

>> Sandhya Viswanathan has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Change method name
>
> Hi @sviswa7 , some comments, overall patch looks good to me.
> 
> Best Regards,
> Jatin

Thanks a lot @jatin-bhateja. I have implemented your review comments.

> src/hotspot/share/opto/vectorIntrinsics.cpp line 772:
> 
>> 770: 
>> 771:   if (elem_klass == nullptr || shuffle_klass == nullptr || shuffle->is_top() || vlen == nullptr) {
>> 772:     return false; // dead code
> 
> Why dead code  in comment ? this is a failed intrinsification condition.

Modified comment.

> src/hotspot/share/opto/vectorIntrinsics.cpp line 776:
> 
>> 774:   if (!vlen->is_con() || shuffle_klass->const_oop() == nullptr) {
>> 775:     return false; // not enough info for intrinsification
>> 776:   }
> 
> Why don't you club it with above conditions to be consistent with other inline expanders ?

Done

> src/hotspot/share/opto/vectorIntrinsics.cpp line 2120:
> 
>> 2118: 
>> 2119:   if (vector_klass == nullptr  || elem_klass == nullptr || vlen == nullptr) {
>> 2120:     return false; // dead code
> 
> Why dead code in comments ?

Modified comment.

> src/hotspot/share/opto/vectorIntrinsics.cpp line 2129:
> 
>> 2127:                     NodeClassNames[argument(2)->Opcode()],
>> 2128:                     NodeClassNames[argument(3)->Opcode()]);
>> 2129:     return false; // not enough info for intrinsification
> 
> Please club this with above condition to be consistent with other inline expanders.

done

> src/hotspot/share/opto/vectorIntrinsics.cpp line 2144:
> 
>> 2142:   int num_elem = vlen->get_con();
>> 2143:   if ((num_elem < 4) || !is_power_of_2(num_elem)) {
>> 2144:     log_if_needed("  ** vlen < 4 or not power of two=%d", num_elem);
> 
> Will num_elem < 4  not be handled by L2149 since we have an implementation limitation to support less than 32-bit shuffle / masks.

Yes that should handle it.

> src/hotspot/share/opto/vectorIntrinsics.cpp line 2171:
> 
>> 2169:     use_predicate = false;
>> 2170:     if(!is_masked_op ||
>> 2171:        (!arch_supports_vector(Op_VectorRearrange, num_elem, elem_bt, VecMaskNotUsed) ||
> 
> Suggestion:
> 
>        (!arch_supports_vector(Op_VectorRearrange, num_elem, elem_bt, VecMaskUseLoad) ||

Here it should be VecMaskNotUsed as this case it using blend to emulate masking. The VecMaskUseLoad case is checked at line 2168.

> src/hotspot/share/opto/vectorIntrinsics.cpp line 2188:
> 
>> 2186: 
>> 2187:   if (v1 == nullptr || v2 == nullptr) {
>> 2188:     return false; // operand unboxing failed
> 
> To be consistent with other expanders please emit proper error for unboxing failure like on following line.
> https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/vectorIntrinsics.cpp#L426

done

> src/hotspot/share/opto/vectorIntrinsics.cpp line 2197:
> 
>> 2195:     mask = unbox_vector(argument(6), mbox_type, elem_bt, num_elem);
>> 2196:     if (mask == nullptr) {
>> 2197:       log_if_needed("  ** not supported: op=selectFrom vlen=%d etype=%s is_masked_op=1",
> 
> Error should an unboxing failure here.

done

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

PR Comment: https://git.openjdk.org/jdk/pull/20634#issuecomment-2362249672
PR Review Comment: https://git.openjdk.org/jdk/pull/20634#discussion_r1767601917
PR Review Comment: https://git.openjdk.org/jdk/pull/20634#discussion_r1767602096
PR Review Comment: https://git.openjdk.org/jdk/pull/20634#discussion_r1767605028
PR Review Comment: https://git.openjdk.org/jdk/pull/20634#discussion_r1767605213
PR Review Comment: https://git.openjdk.org/jdk/pull/20634#discussion_r1767607670
PR Review Comment: https://git.openjdk.org/jdk/pull/20634#discussion_r1767610833
PR Review Comment: https://git.openjdk.org/jdk/pull/20634#discussion_r1767615559
PR Review Comment: https://git.openjdk.org/jdk/pull/20634#discussion_r1767617255