Merge lworld

[jatin-bhateja]

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::nof_nonstatic_field() and is agnostic to scalarization, instead it should consider the actual count of inputs fields connected to InlineTypeIR node [12]

F) A minor change in scalarization logic of multi-fields [13]

With above changes in place most of the VectorAPI JTREG test points around vector/shuffle/masks are passing, we plan to fix remaining regressions with subsequent check-ins.

[1] https://github.com/jatin-bhateja/valhalla/blob/84b3b1e11cdef2c870d1f430c7db2ae60f752d5f/src/hotspot/share/oops/inlineKlass.cpp#L541
[2] https://github.com/jatin-bhateja/valhalla/blob/84b3b1e11cdef2c870d1f430c7db2ae60f752d5f/src/java.base/share/classes/java/lang/Class.java#L616
[3] https://github.com/jatin-bhateja/valhalla/blob/84b3b1e11cdef2c870d1f430c7db2ae60f752d5f/src/hotspot/share/classfile/classFileParser.cpp#L1527
[4] https://github.com/jatin-bhateja/valhalla/blob/84b3b1e11cdef2c870d1f430c7db2ae60f752d5f/src/hotspot/share/classfile/classFileParser.cpp#L1644
[5] https://github.com/jatin-bhateja/valhalla/blob/84b3b1e11cdef2c870d1f430c7db2ae60f752d5f/src/hotspot/share/classfile/classFileParser.cpp#L1692
[6] https://github.com/jatin-bhateja/valhalla/blob/84b3b1e11cdef2c870d1f430c7db2ae60f752d5f/src/hotspot/share/classfile/classFileParser.cpp#L1611
[7] https://github.com/jatin-bhateja/valhalla/blob/84b3b1e11cdef2c870d1f430c7db2ae60f752d5f/src/hotspot/share/opto/vectornode.hpp#L1714
[8] https://github.com/jatin-bhateja/valhalla/blob/84b3b1e11cdef2c870d1f430c7db2ae60f752d5f/src/hotspot/share/opto/vectornode.hpp#L1716
[9] https://github.com/jatin-bhateja/valhalla/blob/84b3b1e11cdef2c870d1f430c7db2ae60f752d5f/src/hotspot/share/prims/vectorSupport.cpp#L320
[10] https://github.com/jatin-bhateja/valhalla/blob/84b3b1e11cdef2c870d1f430c7db2ae60f752d5f/src/hotspot/share/prims/vectorSupport.cpp#L323
[11] https://github.com/jatin-bhateja/valhalla/blob/84b3b1e11cdef2c870d1f430c7db2ae60f752d5f/src/hotspot/share/opto/inlinetypenode.cpp#L858
[12] https://github.com/jatin-bhateja/valhalla/blob/84b3b1e11cdef2c870d1f430c7db2ae60f752d5f/src/hotspot/share/opto/inlinetypenode.cpp#L327
[13] https://github.com/jatin-bhateja/valhalla/blob/84b3b1e11cdef2c870d1f430c7db2ae60f752d5f/src/hotspot/share/opto/inlinetypenode.cpp#L44

---

Progress

• Change must not contain extraneous whitespace

Reviewers

• Xiaohong Gong (@XiaohongGong - no project role)

Contributors

• Xiaohong Gong <xgong@openjdk.org>

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/valhalla.git pull/866/head:pull/866
$ git checkout pull/866

Update a local copy of the PR:
$ git checkout pull/866
$ git pull https://git.openjdk.org/valhalla.git pull/866/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 866

View PR using the GUI difftool:
$ git pr show -t 866

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/866.diff

Webrev

Link to Webrev Comment

[jatin-bhateja]

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

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.

[jatin-bhateja]

Could you please rebase this merge with latest lworld, since I merged a fix several days ago on that branch: #860 ?

I do not want to make more changes in this merge, can we can address this in next merge ?

[jatin-bhateja]

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.

[XiaohongGong]

Could you please rebase this merge with latest lworld, since I merged a fix several days ago on that branch: #860 ?

I do not want to make more changes in this merge, can we can address this in next merge ?

Sure, having a next merge is fine to me. Thanks!

[XiaohongGong]

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?

[jatin-bhateja]

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.

[XiaohongGong]

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.

Yes, I agree that adding such an interface can simply many code. However, for the multifield vectorization, it doesn't only need to check the supported vector size, but also the relative ops with the vector size. Do you plan to check these ops as well?

To clean more, I think it's better to add the multifields to the non_static_fields at the stage of ci field parsing based on the vector_size_supported, and so does to the secondary_fields_count just like what I did when we tried to scalarize/vectorize the multifields (see: XiaohongGong@90c5d21#diff-68d9ded9e6db72428f930d6e3d25ad12f804cc3550f7dde862bae0c513358220R97). We have to make sure the InlineTypeNode::fields_count() is the same with the declared nonstatic fields from ciInstanceKlass. So that in the compiler stage, all things are synced and consistent. But I think this is a big refactory to multifields and deserved to revisit in future.

[jatin-bhateja]

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.

Yes, I agree that adding such an interface can simply many code. However, for the multifield vectorization, it doesn't only need to check the supported vector size, but also the relative ops with the vector size. Do you plan to check these ops as well?

Correct, newly introduced interface should check for Load/Store/BroadCast Operations similar to InlineTypeNode::is_multifield_scalarized
https://github.com/jatin-bhateja/valhalla/blob/merge_lworld2/src/hotspot/share/opto/inlinetypenode.cpp#L43

To clean more, I think it's better to add the multifields to the non_static_fields at the stage of ci field parsing based on the vector_size_supported, and so does to the secondary_fields_count just like what I did when we tried to scalarize/vectorize the multifields (see: XiaohongGong@90c5d21#diff-68d9ded9e6db72428f930d6e3d25ad12f804cc3550f7dde862bae0c513358220R97). We have to make sure the InlineTypeNode::fields_count() is the same with the declared nonstatic fields from ciInstanceKlass. So that in the compiler stage, all things are synced and consistent. But I think this is a big refactory to multifields and deserved to revisit in future.

Yes, as mentioned I will revisit ciField side of handling after merge and will add newly discussed interface so that ci* field count is in sync with C2 IR.

We are bookkeeping multifield in a special ciMultifield structure currently and field query APIs based on index / offset traverses this structure. Both offset and index based traversal should perform a linearized walk over this hierarchical structure, since all the FieldInfo objects now contains unique indices.

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

[jatin-bhateja]

/contributor add @XiaohongGong

[openjdk]

@jatin-bhateja
Contributor Xiaohong Gong <xgong@openjdk.org> successfully added.

[XiaohongGong]

Yes, as mentioned I will revisit ciField side of handling after merge and will add newly discussed interface so that ci* field count is in sync with C2 IR.
image

We are bookkeeping multifield in a special ciMultifield structure currently and field query APIs based on index / offset traverses this structure. Both offset and index based traversal should perform a linearized walk over this hierarchical structure, since all the FieldInfo objects now contains unique indices.

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

Make sense to me. Thanks!

[XiaohongGong]

I'm fine with this patch except for some comments I added above and the jtreg crashes. Thanks!

[jatin-bhateja]

I'm fine with this patch except for some comments I added above and the jtreg crashes. Thanks!

Are you still facing JTREG crashes with -Xshare:off ?
I ran tests on AVX512 machine with default setting using above flags and most of the test points are passing as mentioned in PR description in all the *VectorTests.

===============================================
jdk/incubator/vector/Float128VectorTests.java
Total tests run: 2565, Passes: 2562, Failures: 3, Skips: 0
===============================================

Failures are of similar kind around masked vector operations and looks like related to de-optimization.

With UseAVX2 I am also able to reproduces similar issue in stand alone test

// CMD: java -XX:CompileThresholdScaling=0.1 -XX:+TraceDeoptimization -XX:-TieredCompilation -XX:UseAVX=2 -XX:+DeoptimizeALot -Xbatch --add-modules=jdk.incubator.vector -cp . deopt_shuffle_Byte512
import jdk.incubator.vector.*;

class deopt_shuffle_Byte512 {
  public static void leaf(VectorShuffle<?> shuffle) {
     throw new AssertionError(shuffle.toString());
  }
  public static int micro(int [] arr, int ctr) {
      VectorShuffle<?> shuffle = VectorShuffle.fromArray(ByteVector.SPECIES_512, arr, 0);
      if ((ctr % 2500) == 2000) {
          leaf(shuffle);
      }
      return shuffle.laneSource(ctr & (ByteVector.SPECIES_512.length() - 1));
  }
  public static void main(String [] args) {
      long res = 0;
      int [] arr = new int[64];
      for (int i = 0; i < arr.length; i++) {
          arr[i] = 63 - i;
      }
      for (int i = 0; i < 100000; i++) {
          res += micro(arr, i);
      }
      System.out.println("[res] " + res);
  }
}

Exception in thread "main" java.lang.AssertionError: Shuffle[63, 62, 0, 61, 0, 0, 60, 0, 0, 0, 59, 0, 0, 0, 0, 58, 0, 0, 0, 0, 0, 57, 0, 0, 0, 0, 0, 0, 56, 0, 0, 0, 0, 0, 0, 0, 55, 0, 0, 0, 0, 0, 0, 0, 0, 54, 0, 0, 0, 0, 0, 0, 0, 0, 0, 53, 0, 0, 0, 0, 0, 0, 0, 0]
Exception in thread "main" java.lang.AssertionError: Shuffle[63, 62, 61, 60, 59, 58, 57, 56, 55, 54, 53, 52, 51, 50, 49, 48, 47, 46, 45, 44, 43, 42, 41, 40, 39, 38, 37, 36, 35, 34, 33, 32, 31, 30, 29, 28, 27, 26, 25, 24, 23, 22, 21, 20, 19, 18, 17, 16, 15, 14, 13, 12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1, 0]
SHUFFLE_Byte512 FAIL

[XiaohongGong]

Are you still facing JTREG crashes with -Xshare:off ?

Ohh, I forgot this flag. I will rerun with it. Thanks!

Failures are of similar kind around masked vector operations and looks like related to de-optimization.

Yes, I met the same incorrectness issue with deoptimization, and did not get the root cause yet. We can look at this issue once your patch is merged.

[XiaohongGong]

Are you still facing JTREG crashes with -Xshare:off ?

Ohh, I forgot this flag. I will rerun with it. Thanks!

Failures are of similar kind around masked vector operations and looks like related to de-optimization.

Yes, I met the same incorrectness issue with deoptimization, and did not get the root cause yet. We can look at this issue once your patch is merged.

The jtreg crashes are gone with -Xshare::off.

[jatin-bhateja]

Hi @XiaohongGong , I have addresses and responded to all comments. Let me know if its good to merge now.

[jatin-bhateja]

After payload element type for VectorShuffle is changed from byte to the same type with vector, following payload classes and the related codes in hotspot are never used and can be removed:

VectorPayloadMF8B
VectorPayloadMF16B
VectorPayloadMF32B

I have submitted a patch on mainline to backout shuffle related changes citing performance issues JDK-8310459. So these classes may be needed in next mainline merge.

[XiaohongGong]

Looks good to me!

[jatin-bhateja]

/integrate

[openjdk]

Going to push as commit d44617c.

[openjdk]

@jatin-bhateja Pushed as commit d44617c.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.