8386769: [lworld] C2 parts of JDK-8350865 - Part. 2

[marc-chevalier]

This PR replaces the Valhalla draft PR openjdk/valhalla#2653, which was already reviewed by @TobiHartmann but was not integrated due to the code freeze immediately before the Valhalla mainline integration.

---

PhaseIterGVN::add_users_of_use_to_worklist / PhaseCCP::push_cast

https://github.com/openjdk/valhalla/blob/53ef1bfa0951e5cc1eeb064026eb0d73329e7c5e/src/hotspot/share/opto/phaseX.cpp#L2819
https://github.com/openjdk/valhalla/blob/53ef1bfa0951e5cc1eeb064026eb0d73329e7c5e/src/hotspot/share/opto/phaseX.cpp#L3236

I think that is needed, it's not trivial to make fail without it, but reading code, it must be useful.

ciTypeFlow::StateVector::do_aload

https://github.com/openjdk/valhalla/blob/53ef1bfa0951e5cc1eeb064026eb0d73329e7c5e/src/hotspot/share/ci/ciTypeFlow.cpp#L603

I've did that, I think.

ciReplay.cpp

https://github.com/openjdk/valhalla/blob/53ef1bfa0951e5cc1eeb064026eb0d73329e7c5e/src/hotspot/share/ci/ciReplay.cpp#L517

For that, I just check objArray, not only refArray. Getting from a objArray does the dispatch. It shouldn't be harder than that.

https://github.com/openjdk/valhalla/blob/53ef1bfa0951e5cc1eeb064026eb0d73329e7c5e/src/hotspot/share/ci/ciReplay.cpp#L1143-L1145

This one is less direct. So now, I dump the flatness of the array as ref or flat, and the nullability (nullable or null-free). After that, if the array is flat, I add atomicity (atomic or non-atomic). On parsing, I get all these pieces into a an ArrayProperties given to the oopFactory::new_*Array.

TestIntrinsics.java

https://github.com/openjdk/valhalla/blob/53ef1bfa0951e5cc1eeb064026eb0d73329e7c5e/test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestIntrinsics.java#L2038-L2040
https://github.com/openjdk/valhalla/blob/53ef1bfa0951e5cc1eeb064026eb0d73329e7c5e/test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestIntrinsics.java#L2056-L2058

That was already done, it seems:

https://github.com/openjdk/valhalla/blob/53ef1bfa0951e5cc1eeb064026eb0d73329e7c5e/src/hotspot/share/opto/library_call.cpp#L4907-L4910

TestFlatInArraysFolding.java

https://github.com/openjdk/valhalla/blob/53ef1bfa0951e5cc1eeb064026eb0d73329e7c5e/test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestFlatInArraysFolding.java#L81
https://github.com/openjdk/valhalla/blob/53ef1bfa0951e5cc1eeb064026eb0d73329e7c5e/test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestFlatInArraysFolding.java#L168
https://github.com/openjdk/valhalla/blob/53ef1bfa0951e5cc1eeb064026eb0d73329e7c5e/test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestFlatInArraysFolding.java#L285

Since the important part is that classes are more of less flattable on a setting, I've tried to replace FlatArrayElementMaxSize with FlatArrayElementMaxOops.

TestBasicFunctionality.java

https://github.com/openjdk/valhalla/blob/53ef1bfa0951e5cc1eeb064026eb0d73329e7c5e/test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestBasicFunctionality.java#L578-L581

It looks like it works now, but it should be 3, not 2. I've commented why, and I think it's behaving as it should.

TestLWorld.java

https://github.com/openjdk/valhalla/blob/53ef1bfa0951e5cc1eeb064026eb0d73329e7c5e/test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestLWorld.java#L4646

I've quadruplicated the test for different kind of factories. I've also made sure the flatness hypotheses are still matched with nullable arrays, so I needed to shrink the classes.

SubTypeCheckNode::sub

https://github.com/openjdk/valhalla/blob/53ef1bfa0951e5cc1eeb064026eb0d73329e7c5e/src/hotspot/share/opto/subtypenode.cpp#L50

I think it's good actually. We are already using that inside Compile::static_subtype_check, called a bit lower, but we just need a bit of preparation. We could put that in a method of the type system, but for which benefit? I suggest we keep it as it is for now

Parse::array_store

We could, but we didn't do. It would only matter when the array element has no null-free layout (atomic or non-atomic), but the array is not provably non-flat, which means, there is a nullable layout. That seems unlikely at the very least, or rather impossible in the current state of things.

The experimental patch, for record

diff --git a/src/hotspot/share/opto/parse2.cpp b/src/hotspot/share/opto/parse2.cpp
index 1b0b02c53ee..7511237a28f 100644
--- a/src/hotspot/share/opto/parse2.cpp
+++ b/src/hotspot/share/opto/parse2.cpp
@@ -253,24 +253,41 @@ void Parse::array_store(BasicType bt) {
       // Array might be a flat array, emit runtime checks (for null, a simple inline_array_null_guard is sufficient).
       assert(UseArrayFlattening && !not_flat && elemtype->is_oopptr()->can_be_inline_type() &&
              (!array_type->klass_is_exact() || array_type->is_flat()), "array can't be a flat array");
-      // TODO 8350865 Depending on the available layouts, we can avoid this check in below flat/not-flat branches. Also the safe_for_replace arg is now always true.
-      array = inline_array_null_guard(array, stored_value_casted, 3, true);
+      // If the array cannot be flat and null-free, we don't need to do inline_array_null_guard
+      // in the flat branch under. In this case, the flat branch is thus a bit faster, but the
+      // non-flat branch always need to be protected. If the array can be both flat and null-free,
+      // we just do the inline_array_null_guard guard the flat/non-flat branching so we don't
+      // duplicate it.
+      bool can_be_flat_and_null_free =
+           !elemtype->is_inlinetypeptr()  // Can't tell statically whether it's possible, so we must soundly assume so
+        || elemtype->inline_klass()->has_null_free_atomic_layout()
+        || elemtype->inline_klass()->has_null_free_non_atomic_layout();
+      if (can_be_flat_and_null_free) {
+        array = inline_array_null_guard(array, stored_value_casted, 3, true);
+        array_type = _gvn.type(array)->is_aryptr();
+      }
       // Reload array type which could have been updated by inline_array_null_guard().
-      array_type = _gvn.type(array)->is_aryptr();
       IdealKit ideal(this);
       ideal.if_then(flat_array_test(array, /* flat = */ false)); {
         // Non-flat array
         if (!array_type->is_flat()) {
           sync_kit(ideal);
-          assert(array_type->is_not_flat() || ideal.ctrl()->in(0)->as_If()->is_flat_array_check(&_gvn), "Should be found");
+          Node* array_ = array;
+          const TypeAryPtr* array_type_ = array_type;
+          if (!can_be_flat_and_null_free) {
+            array_ = inline_array_null_guard(array_, stored_value_casted, 3, false);
+            array_type_ = _gvn.type(array_)->is_aryptr();
+          }
+          assert(array_type_->is_not_flat() || ideal.ctrl()->in(0)->as_If()->is_flat_array_check(&_gvn), "Should be found");
           inc_sp(3);
-          access_store_at(array, adr, adr_type, stored_value_casted, elemtype, bt, MO_UNORDERED | IN_HEAP | IS_ARRAY, false);
+          access_store_at(array_, adr, adr_type, stored_value_casted, elemtype, bt, MO_UNORDERED | IN_HEAP | IS_ARRAY, false);
           dec_sp(3);
           ideal.sync_kit(this);
         }
       } ideal.else_(); {
         // Flat array
         sync_kit(ideal);
+        // Either array can be null-free and flat, and inline_array_null_guard is done above, or it cannot, and we don't need to perform this check.
         if (!array_type->is_not_flat()) {
           // Try to determine the inline klass type of the stored value
           ciInlineKlass* vk = nullptr;

At least, we can remove the last parametert of inline_array_null_guard since we are not doing this change.

GraphKit::null_free_atomic_array_test

https://github.com/openjdk/valhalla/blob/53ef1bfa0951e5cc1eeb064026eb0d73329e7c5e/src/hotspot/share/opto/graphKit.cpp#L3985

It's a comment from a time with less testing. Let's not add one more

LibraryCallKit::inline_unsafe_flat_access

This seems to be a bigger piece. I've filed JDK-8388444 for now.

Thanks,
Marc

---

• I confirm that I make this contribution in accordance with the OpenJDK Interim AI Policy.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

• JDK-8386769: [lworld] C2 parts of JDK-8350865 - Part. 2 (Sub-task - P4)

Reviewers

• Tobias Hartmann (@TobiHartmann - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 32146

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/32146.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back mchevalier! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[openjdk]

⚠️ @marc-chevalier This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

[openjdk]

@marc-chevalier The following label will be automatically applied to this pull request:

• hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[openjdk]

The total number of required reviews for this PR has been set to 2 based on the presence of this label: hotspot-compiler. This can be overridden with the /reviewers command.

[mlbridge]

Webrevs

• 00: Full (2dbce4e0)

[TobiHartmann]

Still good!