[lworld+vector] RFR: 8304310: Initial compilers and runtime handling for multifield backed vectors. [v13]

Sat May 6 01:07:42 UTC 2023

On Thu, 4 May 2023 06:52:35 GMT, Jatin Bhateja <jbhateja at openjdk.org> wrote:

>> Please find below the summary of changes included with the patch:
>>   
>> a) _ci Model:_
>>    oops model creates separate fieldDescriptor/FieldInfo for each scalar field of a multifield bundle, root field is marked as multifield_base and non -  root (synthetic) fileds carry a multifield flag. To ease vector IR construction ci only exposes base multifield and ciType holding bundle size to compilers.
>> 
>> b) _C1 compiler:_
>>    Special handling to copy entire multifield bundle during withfield bytecode processing.
>> 
>> c) _C2 compiler:_
>>    VectorBox becomes a derivative of InlineType IR node, changes in vector box/unbox expansions. Preventing scalarization of Vector arguments and return values.
>> 
>> d) _Runtime:_
>>    Changes in object re-construction during deoptimization since now concrete vectors have multifield based payloads. Payload (VectorPayloadMF) is a primitive class object which gets fully flattened within its parent/holding instance i.e. a concrete vector, special handling has been added for non-flattened case where payload is allocated over heap and concrete vector payload field is assigned its reference.
>> 
>> e) Added type specific multifield payloads to ease vector IR creation by C2, this will ensure bundle type and C2 IR type are compatible.               
>> 
>> Scope of current changes is limited to Vector types only, shuffles and masks are still backed by array based payloads. This PoC patch has been validated over vector only tests. More robust validation will be done in due course.
>> 
>> More information can be found at following link
>> [https://cr.openjdk.org/~jbhateja/vectorIntrinsics/Valhalla/lworld%2Bvector.pptx](https://cr.openjdk.org/~jbhateja/vectorIntrinsics/Valhalla/lworld%2Bvector.pptx)
>> 
>> Please share your feedback and suggestions.
>> 
>> Best Regards,
>> Jatin
>
> Jatin Bhateja has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Handling multifield access through Unsafe.get API, some more cleanups.

Impressive work, Jatin!

src/hotspot/cpu/x86/interp_masm_x86.cpp line 1189:

> 1187:     movptr(rscratch1, rax);
> 1188:     // Skip scalarization for vector value objects (concrete vectors and payloads).
> 1189:     super_call_VM_leaf(CAST_FROM_FN_PTR(address, SharedRuntime::is_vector_value_instance), rdi);

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?

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).

I see `InlineKlass::can_be_returned_as_fields()` but don't fully understand how it interacts with interpreter.

src/hotspot/share/c1/c1_GraphBuilder.cpp line 1866:

> 1864:   for (int i = 0; i < vk->nof_nonstatic_fields(); i++) {
> 1865:     ciField* inner_field = vk->nonstatic_field_at(i);
> 1866:     for (int j = 0, sec_offset = 0; j < inner_field->secondary_fields_count(); j++) {

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).

src/hotspot/share/ci/ciField.hpp line 112:

> 110:   ciType* type() { return (_type == NULL) ? compute_type() : _type; }
> 111: 
> 112:   bool is_multifield() { return _is_multifield; }

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. 

The scenario I'm specifically concerned about is when vector and scalar accesses are mixed. Is it possible when Unsafe API is used? 

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.

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()`?

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);

src/hotspot/share/prims/vectorSupport.cpp line 217:

> 215:         int off   = i % VMRegImpl::stack_slot_size;
> 216:         address elem_addr = reg_map->location(vreg, vslot) + off; // assumes little endian element order
> 217:         obj->byte_field_put(ffo + i, *(jbyte*)elem_addr);

It would be nicer to iterate over secondary fields of the multifield and copy one field at a time (as `init_payload_element` does for primitive arrays). But it's also fine to copy byte-by-byte for now.

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.

src/hotspot/share/prims/vectorSupport.cpp line 403:

> 401:   fieldDescriptor fd;
> 402:   int elem_size = type2aelembytes(elem_bt);
> 403:   Symbol* payload_sig = VectorSupport::get_vector_payload_field_signature(elem_bt, num_elem);

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?

src/hotspot/share/prims/vectorSupport.hpp line 34:

> 32: #include "runtime/registerMap.hpp"
> 33: #include "utilities/exceptions.hpp"
> 34: #include "ci/ciKlass.hpp"

`prims/vectorSupport.hpp` not the right place for CI-related queries.

src/hotspot/share/runtime/deoptimization.cpp line 1146:

> 1144:         if (EnableVectorSupport && VectorSupport::is_vector(ik)) {
> 1145:           obj = VectorSupport::allocate_vector(ik, fr, reg_map, sv, THREAD);
> 1146:         } else if (EnableVectorSupport && VectorSupport::is_vector_payload_mf(ik)) {

Why do you need to handle both cases now? Anything special needed for `VectorSupport::is_vector()` case?

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.

src/jdk.incubator.vector/share/classes/jdk/incubator/vector/Byte128Vector.java line 75:

> 73:     }
> 74: 
> 75:     static final Byte128Vector ZERO = new Byte128Vector(VectorPayloadMF.createVectPayloadInstance(byte.class, 16));

You could allocate `VectorSupport.VectorPayloadMF128B` directly or just use default value. 

Not sure `VectorPayloadMF.createVectPayloadInstance()` makes the intention clearer.

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

PR Review: https://git.openjdk.org/valhalla/pull/833#pullrequestreview-1415559103
PR Review Comment: https://git.openjdk.org/valhalla/pull/833#discussion_r1186570892
PR Review Comment: https://git.openjdk.org/valhalla/pull/833#discussion_r1186573143
PR Review Comment: https://git.openjdk.org/valhalla/pull/833#discussion_r1186572111
PR Review Comment: https://git.openjdk.org/valhalla/pull/833#discussion_r1186573359
PR Review Comment: https://git.openjdk.org/valhalla/pull/833#discussion_r1186573836
PR Review Comment: https://git.openjdk.org/valhalla/pull/833#discussion_r1186569846
PR Review Comment: https://git.openjdk.org/valhalla/pull/833#discussion_r1186568598
PR Review Comment: https://git.openjdk.org/valhalla/pull/833#discussion_r1186567323
PR Review Comment: https://git.openjdk.org/valhalla/pull/833#discussion_r1186566286
PR Review Comment: https://git.openjdk.org/valhalla/pull/833#discussion_r1186565115
PR Review Comment: https://git.openjdk.org/valhalla/pull/833#discussion_r1186555380
PR Review Comment: https://git.openjdk.org/valhalla/pull/833#discussion_r1186564209