RFR: 8309531: Incorrect result with unwrapped iotaShuffle. [v6]

[Tobias Hartmann]

On Mon, 3 Jul 2023 07:51:20 GMT, Jatin Bhateja <jbhateja at openjdk.org> wrote:

>> Patch fixes following two issues in iotaShuffle inline expander with unwrapped indices.
>>  1) Disable intrinsification if effective index do not lie within byte value range.
>>  2) Use GT predicate while computing comparison mask for all the indices above vector length.
>> 
>> No performance degradation seen with existing slice/unslice operations which internally calls wrapped iotaShuffle.
>> 
>> This interim patch addresses incorrectness around iotaShuffle till we introduce modified shuffle implementations
>> with JDK-8310691.
>> 
>> Kindly review and share feedback.
>> 
>> Best Regards,
>> Jatin
>
> Jatin Bhateja has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Review comments resolution.

All tests passed, I added some style comments.

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

> 593:   const TypeInt*     wrap          = gvn().type(argument(6))->isa_int();
> 594: 
> 595:   if (shuffle_klass == nullptr || shuffle_klass->const_oop() == nullptr     ||

Suggestion:

  if (shuffle_klass == nullptr || shuffle_klass->const_oop() == nullptr ||

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

> 618: 
> 619:   if (!do_wrap && !effective_indices_in_range) {
> 620:     // FIXME: disable instrinsification for unwrapped shuffle iota

Please don't add FIXME's to new code or at least file a follow-up RFE and reference it here.

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

> 639: 
> 640:   bool step_multiply = !step_val->is_con() || !is_power_of_2(step_val->get_con());
> 641:   if(step_multiply) {

Suggestion:

  if (step_multiply) {

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

> 644:     }
> 645:   } else {
> 646:     if (!arch_supports_vector(Op_LShiftVB, num_elem, elem_bt, VecMaskNotUsed)) {

Can be converted to `else if`

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

> 657:   Node* step  = argument(5);
> 658: 
> 659:   if(step_multiply) {

Suggestion:

  if (step_multiply) {

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

PR Review: https://git.openjdk.org/jdk/pull/14700#pullrequestreview-1512123128
PR Review Comment: https://git.openjdk.org/jdk/pull/14700#discussion_r1251530503
PR Review Comment: https://git.openjdk.org/jdk/pull/14700#discussion_r1251529875
PR Review Comment: https://git.openjdk.org/jdk/pull/14700#discussion_r1251531381
PR Review Comment: https://git.openjdk.org/jdk/pull/14700#discussion_r1251532043
PR Review Comment: https://git.openjdk.org/jdk/pull/14700#discussion_r1251530845