[lworld+vector] RFR: Merge lworld

Jatin Bhateja jbhateja at openjdk.org
Mon Jun 26 05:49:33 UTC 2023


On Thu, 22 Jun 2023 06:47:31 GMT, Jatin Bhateja <jbhateja at openjdk.org> wrote:

> Merge from lworld in lworld+vector, JDK-8292818 made significant modification to FieldInfo by replacing 96 bits fixed set of metadata fields with UNSIGNED5 compressed fields. 
> 
> Other Fixes addressed:-
> 
> A) CDS related: 
>   - All the _VectorPayloadMF*_ classes are part of java.base modules and thus gets archived into a classes.jsa. 
>   - Code for bookkeeping InlineKlassFixedBlock pointer containing  packing/unpacking adapters, exact bundle size etc. is removed from lworld branch, this causes problem in down stream flow accessing those fields.
>   
>   - InstanceKlass::size method takes into account size of InlineKlassFixedBlock and therefore its contents are already part of shared archive, reinitialized the pointer by making an explicit call to _inlineklass_static_block()_ [1] during unshared field restoration.	
> 
>   - Java class structure [2] contains primaryType (pointer to primitive klass mirrors) and secondaryTypes (pointer to primitive values mirrors), these are no-longer getting restored during shared archive loading at VM startup. These pointer are used for class level equality comparison. As a workaround, using -Xshare:off flag during regression testing, this will be fixed in a follow up patch. 
> 
> B) Removed redundant ResourceMarks during field parsing [3], this was resulting into crashes during re-allocations of  __temp_field_info_ (a GrowableArray) due to __nesting_ level mismatch b/w actual allocation site and re-allocation sites. Re-allocations happens because upfront space reservation for growable array do not take into account multifield bundle size. 
> 
> C) JDK-8292818 added a new __index_ field on top regular fields specified in JVM specification (name_index, signature_index, flags_index, access_index) into FieldInfo structure, handled its initialization while parsing multifield[4], injected[5] and user defined fields[6]. 
> 
> D) Small change in VectorBox creation routine for proper initialization[7] of _declared_nonstatic_fields before accessing it[8]. 
> 
> E) De-optimization related:-
>   - De-coupled[9] allocation and re-assignment during vector box re-materialization leveraging existing re-assignment framework[10]. This prevents introducing special handlings for non-vector Locations in existing routines since an InlineTypeNode may have scalar / vector / constant inputs (scalarized multifield bundle initialization during make_default [11]).
> 
>   - nfield count which determines number of field level bookkeeping at SafePoint was initialized using _ciInlineKlass::...

> > > For the scalarization changes on multifields https://github.com/jatin-bhateja/valhalla/blob/84b3b1e11cdef2c870d1f430c7db2ae60f752d5f/src/hotspot/share/opto/inlinetypenode.cpp#L327, you'v assumed that multifields are used for `vector_payload` or `vector_payload_mf` classes. Although it is true currently, it will be more reasonable if we scalarize the `InlineTypeNode` without such assumptions. What about if multifields are used elsewhere?
> > 
> > 
> > Yes, I plan to address it after this merge, currently there is a discrepancy b/w field count returned by _ciInstanceKlass::nof_nonstatic_fields()_ vs InlineTypeNode::field_count(), this is because ci* data model directly works over oops data model which is populated by classFileParser, thus a multifield is a regular field @multifield annotation, its only during c2 parsing we scalarize it.
> > We will need to add a new interface to query supported vector size in ciInstance which is implemented by both the compilers, this will be used to return correct field count which matches with IR.
> 
> Could you please take a look at the similar changes in https://github.com/openjdk/valhalla/pull/863/files#diff-b0ea998b1ae6491c87dae54a2a70644f8e957e7f3522f780883d72fb29107aea, which the compiler firstly get the `nof_nonstatic_fields()`, and then extend the fields by the secondary fields once the field is a multifield_base. Adding another interface in `ciInstanceKlass` sounds ok, but I don't think it's a good idea creating one more similar interface. It already has `nof_nonstatic_fields` and `nof_declared_nonstatic_fields` now.
> 
> If the changes under `hotspot/share/opto` in #863 looks fine to you, I can rebase it once this PR is merged, or you integrate it in this PR. Then I will abandon the 863. WDYT?

Yes, I grepped though the code and currently there are not many occurrences of _"nof_declared_nonstatic_fields"_ and your patch #863 is addressing them,  But having consistency b/w field count returned by ci* object model and C2 IR is also desirable.
BTW, its only C2 which creates Vector IR currently, hence adding a new API  _ciEnv::supports_vector_size(int num_elem, BasicType elem_bt)_  which is implemented by both C1 (returning false) and C2 using _Matcher::vector_size_supported_ may cleanup some code from your patch, WDYT ?

All the ci* query APIs which returns ciField based on offset / index have already been extended to support multifield. But I plan to revisit them after merge patch gets integrated.

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

PR Comment: https://git.openjdk.org/valhalla/pull/866#issuecomment-1606692773



More information about the valhalla-dev mailing list