[lworld+vector] RFR: 8307715: Integrate VectorMask with value/primitive classes [v2]

[Jatin Bhateja]

On Wed, 17 May 2023 09:07:18 GMT, Xiaohong Gong <xgong at openjdk.org> wrote:

>> This patch changes the Vector API java side and hotspot
>> compiler code to make the `VectorMask` operations work well
>> after integrating with valhalla value/primitive classes.
>> 
>> Java side changes include:
>>  - Define the concrete VectorMask classes as value class.
>>  - Define the payload field type as primitive classes, which
>>    defines `MultiField` annotated field to save the element
>>    values.
>>  - Change all the relative API implementations, which use
>>    Unsafe APIs to access the new payload.
>>  - Move several VectorMask default operations to the abstract
>>    super class.
>> 
>> Compiler changes include:
>>  - Enable intrinsification for VectorMask related operations.
>>    Mask input/output of `VectorBox/VectorUnbox` is changed to
>>    the boolean vector format, to adapt C2's `InlineTypeNode`
>>    and new `VectorBox` features. Note that the mask input/output
>>    is a normal vector or predicate before.
>>  - Refine `VectorBox` expanding logic to make it right when the
>>    primitive payload instance is not flattened.
>>  - Minor code cleanup.
>> 
>> Basic VectorMask jtreg tests pass with
>> "`-XX:+UnlockExperimentalVMOptions -XX:-EnableVectorSupport`",
>> and the basic unit tests work well after enabling vector support.
>
> Xiaohong Gong has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix issue when the boxed type is not equal to the intrinsic return type

Your patch looks good to me apart from some minor comments.

src/hotspot/share/opto/castnode.cpp line 99:

> 97:       return vt;
> 98:     }
> 99:   }

As per[ JEP ](https://openjdk.org/jeps/8277163) Value classes (primitve and value) are implicitly final. Also both oop and InlineTypeNode should always be of same type. Do you see a case where a cast should be applied to oop but InlineTypeNode remain sacrosanct ?
Only  one I can think off is when InlineTypeNode was created using _make_default_ / _make_uninitialized_ and _ciInstanceKlass_ was uninitialized

src/hotspot/share/opto/vector.cpp line 248:

> 246:   // Multi-field based vectors are InlineTypeNodes and are already
> 247:   // scalarized by process_inline_types.
> 248:   if (is_vector(iklass) || is_vector_mask(iklass)) {

We can directly use _!is_vector_shuffle(iklass)_

src/hotspot/share/opto/vector.cpp line 495:

> 493: // Since intrinsification is skipped upfront for mask/shuffle related operations
> 494: // this is anyways a dead code currently.
> 495: void PhaseVector::expand_vunbox_node_shuffle(VectorUnboxNode* vec_unbox) {

You can also remove mask related functionality from this routines, since you have already moved it to box creation method.

src/hotspot/share/opto/vector.cpp line 575:

> 573:     ciInstanceKlass* from_kls = tinst->instance_klass();
> 574: 
> 575:     if (is_vector(from_kls) || is_vector_mask(from_kls)) {

Same as above, we can directly use _!is_vector_shuffle(from_kls)_

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

> 49: 
> 50:   ciInstanceKlass* ik = vbox_type->instance_klass();
> 51:   assert(is_vector(ik) || is_vector_mask(ik), "not a vector or a vector mask");

Same as above.

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

> 181:   }
> 182:   // TODO[valhalla] Limiting support to only vector and vector mask cases untill shuffle becomes inline types.
> 183:   if (!is_vector(vbox_type->instance_klass()) && !is_vector_mask(vbox_type->instance_klass())) {

Same as above.

src/jdk.incubator.vector/share/classes/jdk/incubator/vector/Double64Vector.java line 642:

> 640:             return VectorSupport.binaryOp(VECTOR_OP_AND, Double64Mask.class, null,
> 641:                                           long.class, VLENGTH, this, m, null,
> 642:                                           (m1, m2, vm) -> (Double64Mask) m1.bOp(m2, (i, a, b) -> a & b));

There is difference in naming convention of fallback routine b/w masks (bOp) and vectors (bOpMF),  we should have a one consistent convention.

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

PR Review: https://git.openjdk.org/valhalla/pull/845#pullrequestreview-1432103584
PR Review Comment: https://git.openjdk.org/valhalla/pull/845#discussion_r1197438355
PR Review Comment: https://git.openjdk.org/valhalla/pull/845#discussion_r1197442724
PR Review Comment: https://git.openjdk.org/valhalla/pull/845#discussion_r1197455539
PR Review Comment: https://git.openjdk.org/valhalla/pull/845#discussion_r1197451664
PR Review Comment: https://git.openjdk.org/valhalla/pull/845#discussion_r1197452188
PR Review Comment: https://git.openjdk.org/valhalla/pull/845#discussion_r1197462200
PR Review Comment: https://git.openjdk.org/valhalla/pull/845#discussion_r1197483541