[lworld+vector] RFR: 8304310: Initial compilers and runtime handling for multifield backed vectors. [v13]
Jatin Bhateja
jbhateja at openjdk.org
Tue May 9 19:08:05 UTC 2023
On Sat, 6 May 2023 00:43:29 GMT, Vladimir Ivanov <vlivanov at openjdk.org> wrote:
> I'm curious what's the current way to disable scalarization for return values of primitive types other than vector. Or do all primitive classes unconditionally scalarized irrespective of how many elements they comprise?
>
Currently entire handling operates under two global flags `InlineTypeReturnedAsFields` and `InlineTypePassFieldsAsArgs`
I introduced special handling to prevent scalarization for vectors in `InlineKlass::can_be_passed_as_fields` and `InlineKlass::can_be_returned_as_fields`.
> It makes sense to introduce a flag on a Klass which signals when return value scalarization should be turned off (unless there's already such a flag present).
Agree, I can handle this on lworld branch since it will be good to enhance core infrastructure to disable scalarization per class basis even though value type semantics always enforces scalarization, vector values are special since multifield needs to be passed using vector and we do not support a mixed GPR+VECTOR calling convention currently.
>
> I see `InlineKlass::can_be_returned_as_fields()` but don't fully understand how it interacts with interpreter.
InlineKlass contains unpack / pack handlers , interpreter and c1 side handling directly operate under influence of above mentioned global flags. There is no mechanism to prevent scalarization for value objects currently, apart from c2 all the other execution engines uses pack/unpack handlers to pass / receive a value object.
> Does it make sense to expose through CI a flat view over all fields which filters out multifields? (Also, relates to my comment later about aliasing in C2).
Currently, I create a hierarchical field structure with all synthetic multifield hidden beneath base multi-field and are exposed to C2 only if target does not support vector size to load multi-field.
> I'm concerned about how multi-field and the fields it consists of interact at runtime and affect C2 alias analysis which is offset-based.
>
We always access multi-field through unsafe APIs, thus the alias are computed based on address calculation over payload class objects. Both payload (VectorPayloadMF*) and computed address are of TypeInstPtr type. This is different from regular array element update which gets captured in MemoryMergeNode at same alias index as of entire array.
> The scenario I'm specifically concerned about is when vector and scalar accesses are mixed. Is it possible when Unsafe API is used?
>
Unsafe.put operate over buffered InlineTypeNodes followed by re-materialization of InlineTypeNode, and a Unsafe.get operation loads particular field from buffer, their effective addresses are computed with payload as the base field.
> I'd prefer to see CI to expose either a single multifield (for C2 w/ proper vector support) or a pack of scalar fields (for C1 or C2 w/o proper vector support), but not both at the same time. I believe C1 doesn't care about multifields while C2 should not care about scalar elements when vectors of proper size are supported.
Yes, CI captures both base and synthetic multi-fields and expose the synthetic multi-field for scalar IR creation only if target does not support the vector size.
> src/hotspot/share/ci/ciInlineKlass.cpp line 100:
>
>> 98: // Can this inline type be returned as multiple values?
>> 99: bool ciInlineKlass::can_be_returned_as_fields() const {
>> 100: GUARDED_VM_ENTRY(return !VectorSupport::skip_value_scalarization(const_cast<ciInlineKlass*>(this)) && to_InlineKlass()->can_be_returned_as_fields();)
>
> Why not make vector-specific check a part of `InlineKlass::can_be_returned_as_fields()`?
DONE
> src/hotspot/share/opto/compile.cpp line 2770:
>
>> 2768: TracePhase tp("", &timers[_t_vector]);
>> 2769: PhaseVector pv(igvn);
>> 2770: pv.optimize_vector_boxes();
>
> `PhaseVector` does perform `PhaseRemoveUseless` (as part of `PhaseVector::do_cleanup()`).
>
> How does it interact with `remove_root_to_sfpts_edges` happening earlier:
>
> // Now that all inlining is over and no PhaseRemoveUseless will run, cut edge from root to loop
> // safepoints
> remove_root_to_sfpts_edges(igvn);
Fixed, it was an left behind artifact of a previous check-in, its not moved after PhaseVector completion like in current implementation.
> src/hotspot/share/prims/vectorSupport.cpp line 286:
>
>> 284: }
>> 285:
>> 286: InstanceKlass* VectorSupport::get_vector_payload_klass(BasicType elem_bt, int num_elem) {
>
> Alternatively, it could be implemented as a class lookup by name.
>
> It is fine for now, but eventually I'd like to get rid of `vmSymbols::vector_VectorPayloadMF*_signature()` and `vector_VectorPayloadMF*_klass` and use only `VectorSupport.VectorPayloadMF` to mark classes which need special handling.
>
> Original implementation requires subclasses to declare 2 static fields which are used by `klass2length` and `klass2bt` queries. Since typed vector classes declare a field of primitive type, it can be used as the source of information now.
I thought about it but any lookup by name may be costly. But we can refine as suggested.
> Alternatively, you could derive that information from `payload` field. Would it also make `klass2length()` and `klass2bt()` queries (and relevant static final fields on Java side) obsolete?
find_field need field signature to query the fieldDescriptor. payload is no longer a primitive array but a primitive class instance, we still need pass klass2length() and klass2bt() information to payload signature factory method.
> src/hotspot/share/runtime/sharedRuntime.cpp line 642:
>
>> 640:
>> 641: JRT_LEAF(jint, SharedRuntime::is_vector_value_instance(InlineKlass* klass))
>> 642: return (jint)VectorSupport::skip_value_scalarization(klass);
>
> The naming is confusing here: `is_vector_value_instance` wraps `skip_value_scalarization`. I suggest to use `is_vector_value_instance` uniformly.
DONE
-------------
PR Review Comment: https://git.openjdk.org/valhalla/pull/833#discussion_r1189025540
PR Review Comment: https://git.openjdk.org/valhalla/pull/833#discussion_r1189025720
PR Review Comment: https://git.openjdk.org/valhalla/pull/833#discussion_r1189025613
PR Review Comment: https://git.openjdk.org/valhalla/pull/833#discussion_r1189025791
PR Review Comment: https://git.openjdk.org/valhalla/pull/833#discussion_r1189025872
PR Review Comment: https://git.openjdk.org/valhalla/pull/833#discussion_r1189025435
PR Review Comment: https://git.openjdk.org/valhalla/pull/833#discussion_r1189025348
PR Review Comment: https://git.openjdk.org/valhalla/pull/833#discussion_r1189025000
More information about the valhalla-dev
mailing list