On Thu, 19 Sep 2024 07:29:11 GMT, Jatin Bhateja <jbhat...@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
