8357474: [lworld] Consolidate load/store flat inline types #1470

merykitty · 2025-05-21T16:07:15Z

Hi,

This patch consolidates loading and storing of flat inline types. This makes it so that make_from_flat and store_flat will take a pointer to the target location. make_from_flat_array and store_flat_array are split out into their own methods which receive an idx parameter. This also fixes some issues I theorycrafted:

Improper loading/storing of atomic elements in a non-atomic container. This is not supported at the moment but it would be great to prepare for it.
is_naturally_atomic checks nof_declared_nonstatic_fields, which is incorrect if the sole field has multiple fields. I refactored it into ciInlineKlass, the decision to do atomic loads/stores is also delegated to the callee, the caller only needs to know whether the operations act as if they are atomic, not whether they are actually executed atomically.

null_free of an oop can only be trusted if the container itself is null-free. For example this case:

  value class StringHolder {
      @NullRestricted
      String s;
  }

  value class StringHolderHolder {
      // flatten
      StringHolder v;
  }

Then v.s cannot be trusted to be non-null because v can be null.

This also allows straightforward implementation of https://bugs.openjdk.org/browse/JDK-8349110.

Please let me know what you think. Thanks very much.

Progress

Change must not contain extraneous whitespace

Issue

JDK-8357474: [lworld] Consolidate load/store flat inline types (Bug - P4)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1470

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-05-21T16:08:16Z

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

openjdk · 2025-05-21T16:08:25Z

@merykitty This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8357474: [lworld] Consolidate load/store flat inline types

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 1 new commit pushed to the lworld branch:

be14683: 8342488: [lworld] compiler/c2/irTests/scalarReplacement/ScalarReplacementWithGCBarrierTests.java fails after merging jdk-24+13

Please see this link for an up-to-date comparison between the source branch of this pull request and the lworld branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@TobiHartmann) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

mlbridge · 2025-05-21T16:12:24Z

Webrevs

TobiHartmann

This is a nice refactoring, thanks for working on this!

I'll review the code in detail soon but I submitted some testing already which revealed an issue:

compiler/valhalla/inlinetypes/TestArrayNullMarkers.java
-Xcomp -XX:-TieredCompilation -DIgnoreCompilerControls=true

# A fatal error has been detected by the Java Runtime Environment:
#
#  Internal Error (/workspace/open/src/hotspot/share/opto/memnode.cpp:3587), pid=2958342, tid=2958358
#  assert(Opcode() == st->Opcode() || st->Opcode() == Op_StoreVector || Opcode() == Op_StoreVector || st->Opcode() == Op_StoreVectorScatter || Opcode() == Op_StoreVectorScatter || phase->C->get_alias_index(adr_type()) == Compile::AliasIdxRaw || (Opcode() == Op_StoreL && st->Opcode() == Op_StoreI) || (Opcode() == Op_StoreI && st->Opcode() == Op_StoreL) || (Opcode() == Op_StoreL && st->Opcode() == Op_StoreN) || (st->adr_type()->isa_aryptr() && st->adr_type()->is_aryptr()->is_flat()) || (is_mismatched_access() || st->as_Store()->is_mismatched_access())) failed: no mismatched stores, except on raw memory: StoreB StoreN
#
# JRE version: Java(TM) SE Runtime Environment (25.0) (fastdebug build 25-lworld5ea-LTS-2025-05-22-0636300.tobias.hartmann.valhalla2)
# Java VM: Java HotSpot(TM) 64-Bit Server VM (fastdebug 25-lworld5ea-LTS-2025-05-22-0636300.tobias.hartmann.valhalla2, mixed mode, compressed oops, compressed class ptrs, g1 gc, linux-amd64)
# Problematic frame:
# V  [libjvm.so+0x167db09]  StoreNode::Ideal(PhaseGVN*, bool)+0x2a9

Current CompileTask:
C2:18680  865   !b        compiler.valhalla.inlinetypes.TestArrayNullMarkers::main (2883 bytes)

Stack: [0x00007f99f351b000,0x00007f99f361b000],  sp=0x00007f99f36160b0,  free space=1004k
Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native code)
V  [libjvm.so+0x167db09]  StoreNode::Ideal(PhaseGVN*, bool)+0x2a9  (memnode.cpp:3587)
V  [libjvm.so+0x185de4d]  PhaseIterGVN::transform_old(Node*)+0xbd  (phaseX.cpp:668)
V  [libjvm.so+0x18530d4]  PhaseIterGVN::optimize()+0xb4  (phaseX.cpp:1054)
V  [libjvm.so+0xb53b16]  Compile::Optimize()+0x326  (compile.cpp:2779)
V  [libjvm.so+0xb5773f]  Compile::Compile(ciEnv*, ciMethod*, int, Options, DirectiveSet*)+0x1fef  (compile.cpp:874)
V  [libjvm.so+0x96db34]  C2Compiler::compile_method(ciEnv*, ciMethod*, int, bool, DirectiveSet*)+0x484  (c2compiler.cpp:142)
V  [libjvm.so+0xb653f8]  CompileBroker::invoke_compiler_on_method(CompileTask*)+0xb58  (compileBroker.cpp:2307)
V  [libjvm.so+0xb665b8]  CompileBroker::compiler_thread_loop()+0x568  (compileBroker.cpp:1951)
V  [libjvm.so+0x10df77b]  JavaThread::thread_main_inner()+0x13b  (javaThread.cpp:774)
V  [libjvm.so+0x1b595c6]  Thread::call_run()+0xb6  (thread.cpp:231)
V  [libjvm.so+0x17c4af8]  thread_native_entry(Thread*)+0x128  (os_linux.cpp:875)

test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestBasicFunctionality.java

merykitty · 2025-05-22T19:01:15Z

The failure is due to complications computing the field offset of an array access. When finding the field corresponding to a field offset, we cannot find the null marker because it is not a field inside the element type. There is still one strange case, though, consider this class:

@LooselyConsistentValue
static value class EmptyContainer {
    @Strict
    @NullRestricted
    MyValueEmpty empty = new MyValueEmpty();
}

Then, the null marker of EmptyContainer is at offset 1 inside the payload. I checked for that case and made it a mismatched access now.

TobiHartmann · 2025-05-26T09:07:24Z

src/hotspot/share/opto/graphKit.cpp

@@ -1877,7 +1877,7 @@ Node* GraphKit::flat_array_element_address(Node*& array, Node* idx, ciInlineKlas
  const TypeAryPtr* arytype = TypeOopPtr::make_from_klass(array_klass)->isa_aryptr();
  arytype = arytype->cast_to_exactness(true);
  arytype = arytype->cast_to_not_null_free(is_not_null_free);
-  array = _gvn.transform(new CheckCastPPNode(control(), array, arytype));
+  array = _gvn.transform(new CheckCastPPNode(control(), array, arytype, ConstraintCastNode::StrongDependency));


Did you hit an issue without this?

In principle, when we cast the array to null-free atomic, the result type depends on 2 dominating tests, which are the null-free test and the atomic test. As a result, we cannot use RegularDependency here, because RegularDependency means that the cast depends only on the test which is its control input.

TobiHartmann · 2025-05-26T09:09:37Z

src/hotspot/share/opto/inlinetypenode.cpp

-      bool mismatched = (decorators & C2_MISMATCHED) != 0;
-      if (base->is_Con() && oop_ptr->is_inlinetypeptr() && !is_array && !mismatched) {
-        // If the oop to the inline type is constant (static final field), we can
-        // also treat the fields as constants because the inline type is immutable.


Why is this not needed anymore?

Because the loads we generate can be constant-folded, there is no need for an explicit path here.

src/hotspot/share/opto/inlinetypenode.cpp

TobiHartmann · 2025-05-26T09:17:01Z

src/hotspot/share/opto/inlinetypenode.cpp

+  if (vk->nof_nonstatic_fields() == 0) {
+    region = new RegionNode(3);
+    region->init_req(1, kit->top());
+    region->init_req(2, kit->top());


Why is the top init needed? Aren't nullptr inputs handled the same way in RegionNode::Ideal etc.?

TobiHartmann · 2025-05-26T09:20:27Z

src/hotspot/share/opto/inlinetypenode.cpp

+    region->init_req(1, kit->top());
+    region->init_req(2, kit->top());
+  } else {
+    region = new RegionNode(4);


For simplicity, can't we always have a RegionNode with 4 inputs here and simply leave in(3) as top/nullptr if we don't need it?

Oh that's right, I forgot about that

TobiHartmann · 2025-05-26T10:18:41Z

src/hotspot/share/opto/parse2.cpp

+          const TypeAryPtr* arytype = TypeOopPtr::make_from_klass(array_klass)->isa_aryptr();
+          arytype = arytype->cast_to_exactness(true);
+          arytype = arytype->cast_to_not_null_free(is_not_null_free);
+          Node* flat_array = gvn().transform(new CastPPNode(control(), array, arytype));


I assume you are not using flat_array_element_address anymore because you don't need the address but only want to cast the array but I think this casting should then be factored out. Also, why don't you need a ConstraintCastNode::StrongDependency here?

That's a good idea, I refactored it into GraphKit::cast_to_flat_array and the element address can be trivially obtained from array_element_address.

TobiHartmann · 2025-05-26T10:35:40Z

src/hotspot/share/opto/inlinetypenode.cpp

      if (!kit->stopped()) {
+        assert(vk->has_non_atomic_layout(), "");
+        // TODO 8350865 Is the conversion to/from payload folded? We should wire this directly.
+        // Also remove the PreserveReexecuteState in Parse::array_load when buffering is no longer possible.


Now that you removed the convert_to_payload hack, this comment should be removed, right? Also, buffering should no longer be possible and the referenced code in Parse::array_load can be removed.

Yes you are right, I have done that.

src/hotspot/share/opto/inlinetypenode.cpp

TobiHartmann · 2025-05-26T12:49:46Z

src/hotspot/share/opto/inlinetypenode.cpp

+    Node* cast = base;
+    Node* ptr = kit->flat_array_element_address(cast, idx, vk, /* null_free */ false, /* not_null_free */ true, /* atomic */ true);
+    if (vk->get_field_by_offset(vk->payload_offset(), false) == nullptr && vk->null_marker_offset_in_payload() != 0) {
+      // TODO 8357612 Weird layout


I don't understand why this would be specific to the layout described in JDK-8357612? Wouldn't we also hit vk->get_field_by_offset(vk->payload_offset(), false) == nullptr when the null marker is at vk->payload_offset() which would be totally fine?

The null marker at vk->payload_offset() is covered by vk->null_marker_offset_in_payload() == 0, am I right?

chhagedorn · 2025-05-26T14:02:50Z

src/hotspot/share/opto/parse2.cpp

+
+          ciArrayKlass* array_klass = ciArrayKlass::make(vk, /* flat */ true, is_null_free, maybe_atomic);
+          const TypeAryPtr* arytype = TypeOopPtr::make_from_klass(array_klass)->isa_aryptr();
+          arytype = arytype->cast_to_exactness(true);


Nice refactoring! Just noticed this here: IIUC, we don't know whether it's null-free or not here but we still set it to exact (was like that before already) which we should probably not? But it looks like the way we later use the CastPP in make_from_flat_array(), it does not cause problems because we will feed it into a nullable/not-null-free and null-free cast which is correctly set as exact.

That's a nice catch, I have updated it to cast to exactness only when is_null_free || is_not_null_free

merykitty · 2025-05-26T15:27:43Z

Thanks for your reviews, I have addressed them. I notice there might be some additional issues here, too:

AddPNode::Value does not work properly with pointers into arrays.
The type system around arrays seems unsound. We don't have a way to express maybe_atomic.

I will investigate these further.

merykitty · 2025-05-31T10:21:55Z

I have reworked the functions a little bit to remove the ptr_type parameters as well as remove the need for special cases for types without a real field at the start of their payload.

merykitty added 2 commits May 21, 2025 09:55

Consolidate load/store of flat values

cd74d47

add tests

09d4dda

openjdk bot added ready rfr labels May 21, 2025

merykitty added 2 commits May 21, 2025 23:36

convert_from_payload

8173ef2

array

99d2269

TobiHartmann suggested changes May 22, 2025

View reviewed changes

test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestBasicFunctionality.java Outdated Show resolved Hide resolved

merykitty added 2 commits May 23, 2025 00:46

fix array field calculations

1029080

fix array of nullable empty type

b151571

merykitty added 3 commits May 23, 2025 12:37

generalize weird layout

77f145d

typo

5ac917f

weird layout

f7e3bd3

TobiHartmann suggested changes May 26, 2025

View reviewed changes

chhagedorn reviewed May 26, 2025

View reviewed changes

merykitty added 2 commits May 26, 2025 22:14

cast_to_flat_array

a364b7a

Merge branch 'lworld' into loadstoreflat

e49bbc8

remove ptr_type parameters

0a945fe

8357474: [lworld] Consolidate load/store flat inline types #1470

Are you sure you want to change the base?

8357474: [lworld] Consolidate load/store flat inline types #1470

Uh oh!

Conversation

merykitty commented May 21, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewing

Uh oh!

bridgekeeper bot commented May 21, 2025

Uh oh!

openjdk bot commented May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mlbridge bot commented May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

TobiHartmann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

merykitty commented May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chhagedorn May 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

merykitty commented May 26, 2025

Uh oh!

merykitty commented May 31, 2025

Uh oh!

Uh oh!

merykitty commented May 21, 2025 •

edited by openjdk bot

Loading

openjdk bot commented May 21, 2025 •

edited

Loading

mlbridge bot commented May 21, 2025 •

edited

Loading

merykitty commented May 22, 2025 •

edited

Loading

chhagedorn May 26, 2025 •

edited

Loading