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

[Xiaohong Gong]

On Wed, 24 May 2023 06:19:45 GMT, Jatin Bhateja <jbhateja at openjdk.org> wrote:

>> Xiaohong Gong has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Fix jtreg failures
>
> src/hotspot/share/opto/cfgnode.cpp line 2574:
> 
>> 2572:         }
>> 2573:         const Type* t = phase->type(n);
>> 2574:         if (n->is_InlineType() && !n->is_VectorBox() && (vk == NULL || vk == t->inline_klass())) {
> 
> We should do an upfront check for VectorBoxes to set can_optimize to false on line#2545 so that we never invoke _push_inline_types_through_ , it creates a new InlineTypeNode whose Oop fields may get stitched to VBAs, anyways we have a seperate routine for Phi handling _merge_through_phi_,

Agree, I will change this as what you'v changed in your attached patch. Thanks!

> 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 seems not related to the box/unbox expanding part. I'v recorded the issues and plan to look at in future. You can also take a look if interested. Thanks!

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

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