RFR: 8266054: VectorAPI rotate operation optimization [v10]

[Jatin Bhateja]

On Fri, 16 Jul 2021 00:52:21 GMT, Sandhya Viswanathan <sviswanathan at openjdk.org> wrote:

>> Jatin Bhateja has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 15 commits:
>> 
>>  - 8266054: Incorporating styling changes based on reviews.
>>  - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8266054
>>  - Merge http://github.com/openjdk/jdk into JDK-8266054
>>  - Merge http://github.com/openjdk/jdk into JDK-8266054
>>  - Merge http://github.com/openjdk/jdk into JDK-8266054
>>  - Merge branch 'JDK-8266054' of http://github.com/jatin-bhateja/jdk into JDK-8266054
>>  - 8266054: Removing redundant teat templates.
>>  - 8266054: Code reorganization for efficient sharing of logic to check rotate operation support on a target platform.
>>  - 8266054: Removing redundant test templates.
>>  - 8266054: Review comments resolution.
>>  - ... and 5 more: https://git.openjdk.java.net/jdk/compare/07e90524...609c4143
>
> src/hotspot/share/opto/vectorIntrinsics.cpp line 84:
> 
>> 82:         arch_supports_vector(Op_OrV, num_elem, elem_bt, VecMaskNotUsed)) {
>> 83:       is_supported = true;
>> 84:     }
> 
> When check_bcast is set, is_supported could be false when replicate is not supported. Is replicate not needed for shift+or sequence?

check_bcast is true only when shift value is a non-constant scalar value, in that case we need to check for broadcasting operation for shift, in all other cases broadcast is not needed.  Constant shift value is an optimizing case since AVX512 offers instructions which directly accept 8bit immediate shift value.

> src/hotspot/share/opto/vectorIntrinsics.cpp line 86:
> 
>> 84:     }
>> 85:     return is_supported;
>> 86: }
> 
> Please add comments here why the Left/Right shift and Or opcodes are being checked here. Also add comments why for left shift we are only checking for int and long left shift opcodes whereas for right shift sub word opcodes are being checked.

Both left and right shifts opcodes are selected for all integral types (byte/short/int/long). VectorNode::opcode returns the granular left shift type based on the sub-type i.e. elem_bt in case of  LeftShiftI.  Re-organizing the code for better readability.

> src/hotspot/share/opto/vectorIntrinsics.cpp line 338:
> 
>> 336:     // TODO When mask usage is supported, VecMaskNotUsed needs to be VecMaskUseLoad.
>> 337:     if ((sopc != 0) &&
>> 338:         !arch_supports_vector(sopc, num_elem, elem_bt, is_vector_mask(vbox_klass) ? VecMaskUseAll : VecMaskNotUsed)) {
> 
> Could we not call arch_supports_vector_rotate from arch_supports_vector?

DONE

> src/hotspot/share/opto/vectorIntrinsics.cpp line 1563:
> 
>> 1561:                          -0x80 <= cnt_type->get_con() && cnt_type->get_con() < 0x80;
>> 1562:   if (is_rotate) {
>> 1563:     if (!arch_supports_vector_rotate(sopc, num_elem, elem_bt, !is_const_rotate)) {
> 
> What is the relationship between check_bcast and !is_const_rotate? Some comments here on this would help.

Constant shift value is an optimizing case since AVX512 offers instructions which directly accept constant shifts in the range (-256, 255). Similar handling is done in SLP.
https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/superword.cpp#L2493

But I feel this is very X86 specific check in generic code, so moving decision to a new target specific matcher routine.

> src/hotspot/share/opto/vectorIntrinsics.cpp line 1590:
> 
>> 1588:       opd2 = gvn().transform(VectorNode::scalar2vector(cnt, num_elem, type_bt));
>> 1589:     } else {
>> 1590:       // constant shift.
> 
> Did you mean constant rotate here?

Yes.

> src/hotspot/share/opto/vectornode.cpp line 1180:
> 
>> 1178:       cnt = cnt->in(1);
>> 1179:     }
>> 1180:     shiftRCnt = cnt;
> 
> Why do we remove the And with mask here?

And'ing with shift_mask is already done on Java API side implementation before making a call to intrinsic rountine.

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

PR: https://git.openjdk.java.net/jdk/pull/3720