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

[Jatin Bhateja]

On Thu, 25 May 2023 01:58:35 GMT, Xiaohong Gong <xgong at openjdk.org> wrote:

>> src/hotspot/share/opto/vector.cpp line 308:
>> 
>>> 306:   Node* klass_node = kit.makecon(TypeKlassPtr::make(vk));
>>> 307:   Node* alloc_oop  = kit.new_instance(klass_node, NULL, NULL, /* deoptimize_on_exception */ true, vector);
>>> 308:   vector->store(&kit, alloc_oop, alloc_oop, vk);
>> 
>> Do you think we should explicitly allocate payload for non-flattened case before calling InlineTypeNode::store.
>> As per following 
>> https://github.com/openjdk/valhalla/blob/lworld%2Bvector/src/hotspot/share/classfile/fieldLayoutBuilder.cpp#L768
>> Flattening depends on InlineFieldMaxFlatSize (default 128 bytes so fine for current vectors) but its configurable and value can be reduced.
>
> Yes, allocating a buffer is needed. And the `InlineTypeNode::store()` can handled such cases. If the field type is an inline type and is not flattened, it goes to https://github.com/openjdk/valhalla/blob/lworld%2Bvector/src/hotspot/share/opto/inlinetypenode.cpp#L518, which will create a buffer for the InlineTypeNode. Please see: https://github.com/openjdk/valhalla/blob/lworld%2Bvector/src/hotspot/share/opto/graphKit.cpp#L1656
> 
> I also tested the case with `-XX:InlineFieldMaxFlatSize=0` to force it not be flattened. The VectorBox expanding code is the expected one like before, which includes both the payload buffer and vector buffer and the relative stores. 
> 
> BTW, currently this flag cannot work well since there is issue when handling this option. You may need to apply the following changes when testing:
> 
> diff --git a/src/hotspot/share/classfile/fieldLayoutBuilder.cpp b/src/hotspot/share/classfile/fieldLayoutBuilder.cpp
> index 516ad9bb7..538f31ae6 100644
> --- a/src/hotspot/share/classfile/fieldLayoutBuilder.cpp
> +++ b/src/hotspot/share/classfile/fieldLayoutBuilder.cpp
> @@ -774,7 +774,7 @@ void FieldLayoutBuilder::regular_field_sorting() {
>                //too_volatile_to_flatten = false; //FIXME
>                // volatile fields are currently never inlined, this could change in the future
>              }
> -            if (!(too_big_to_flatten | too_atomic_to_flatten | too_volatile_to_flatten) || fs.access_flags().is_final()) {
> +            if (!(too_big_to_flatten | too_atomic_to_flatten | too_volatile_to_flatten) && fs.access_flags().is_final()) {
>                group->add_inlined_field(fs, vk);
>                _nonstatic_oopmap_count += vk->nonstatic_oop_map_count();
>                fs.set_inlined(true);
> @@ -877,7 +877,7 @@ void FieldLayoutBuilder::inline_class_field_sorting(TRAPS) {
>                //too_volatile_to_flatten = false; //FIXME
>                // volatile fields are currently never inlined, this could change in the future
>              }
> -            if (!(too_big_to_flatten | too_atomic_to_flatten | too_volatile_to_flatten) || fs.access_flags().is_final()) {
> +            if (!(too_big_to_flatten | too_atomic_to_flatten | too_volatile_to_flatten) && fs.access_flags().is_final()) {
>                group->add_inlined_field(fs, vk);
>                _nonstatic_oopmap_count += vk->nonstatic_oop_map_count();
>                field_alignment = vk->get_alignment();
> 
> 
> Also there are other issues when the payload is not flattened. But it...

Thanks for your explanations, yes for non-flattened case buffering happens underneath _access_store_at_

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

PR Review Comment: https://git.openjdk.org/valhalla/pull/845#discussion_r1205076247