[lworld] RFR: 8372700: [lworld] compiler/c2/irTests/stable/* fail with --enable-preview

Fri Dec 19 09:56:18 UTC 2025

On Thu, 18 Dec 2025 12:01:36 GMT, Marc Chevalier <mchevalier at openjdk.org> wrote:

> Flat accesses to a stable value can be expanded in a non-atomic way if the stable field is already initialized since they are read-only at this point. That allows to make more optimizations, and in particular to replace the access by a constant if it's known at compilation time.
> 
> There are 2 cases. First, flat stable non-array field. In this case, the value is known to be stable if the value is non-null, that is if the null-marker of the said field is 1. If we can find that the null marker has a constant value that is non zero, we expand non-atomically. That is done by finding the field we are trying to get from the offset. From the field, we can find the offset of the null marker, and then the null marker `ciField`, allowing to fetch its value if the holder is known to be a constant oop.
> 
> The other case is stable flat array. In this case, we need to find index of the containing element of the array, then with the offset, we can find the field we are trying to get. Finding the null marker here is a bit more tricky. Let's say we have
> 
> value class MyValue {
>     int x;
> }
> class C {
>     MyValue v;  // assumed flat.
> }
> 
> the null marker for `v` is somewhat a field of `C`, as well as `v.x`. So I can just use `field_value` to get the null marker. But in `MyValue[]`, there isn't a single field for the null marker, but one "field" for each cell of the array, and there isn't a nice containing type in which it lives. The only way to get each piece of the array is by index (or offset). So, I needed specialized functions to access the null marker of a cell given the index/offset.
> 
> I also had to implement of bit of accessors for `ciFlatArray`. First, implement `element_value_by_offset` in `ciFlatArray` since the implementation of `ciArray` (super-class) was used, which computes the index from the provided offset, assuming a size of elements that doesn't take flattening into account as it used only the basic type, and not the layout helper. But also helpers to get a field of the flattened value class in a cell, to allow constant folding to get the constant value in an array.
> 
> The last part of the puzzle, necessary to make constant folding happen (see `Type::make_constant_from_field`), is to say that a field of a flattened inline type field is constant if the containing field if constant. In the words of the previous example, that means `x` is constant in `C` if `v` is strict and final (already there), or if `v` is constant itself. That matches what we do in `void ciField::i...

I had a first quick look at this and left a few comments. Will do a full review after vacation.

I assume the folding should also work for arrays with different layouts, right? I think we need tests for all of them, i.e. all `ValueClass.new*Array()` variants.

src/hotspot/share/ci/ciFlatArray.cpp line 47:

> 45: }
> 46: 
> 47: ciConstant ciFlatArray::check_constant_null_marker_cache(int off) {

Do we really need a cache here?

src/hotspot/share/ci/ciFlatArray.hpp line 59:

> 57:   ciConstant check_constant_null_marker_cache(int off);
> 58:   void add_to_constant_null_marker_cache(int off, ciConstant val);
> 59:   //ciConstant element_value_impl(arrayOop ary, int index, int offset);

Should this be removed?

src/hotspot/share/ci/ciInstance.cpp line 106:

> 104: // Constant value of the null marker.
> 105: ciConstant ciInstance::null_marker_value() {
> 106:   if (!klass()->is_inlinetype()) {

Should this be an assert?

src/hotspot/share/opto/compile.cpp line 2132:

> 2130:     if (n->is_LoadFlat()) {
> 2131:       LoadFlatNode* loadn = n->as_LoadFlat();
> 2132:       bool non_atomic_is_fine = false;

Please add a high level comment here, explaining why we can sometimes access non-atomic.

src/hotspot/share/opto/compile.cpp line 2135:

> 2133:       if (FoldStableValues) {
> 2134:         const Type* base_type = igvn.type(loadn->base());
> 2135:         const TypeOopPtr* oopptr = base_type->isa_oopptr();

Suggestion:

        const TypeOopPtr* oopptr = igvn.type(loadn->base())->isa_oopptr();

Maybe rename `oopptr` to `base_type`.

src/hotspot/share/opto/compile.cpp line 2147:

> 2145:           ciField* nm_field = iklass->get_field_by_offset(field->null_marker_offset(), false);
> 2146:           ciConstant cst = nm_field != nullptr ? holder->field_value(nm_field) : ciConstant() /* invalid */;
> 2147:           non_atomic_is_fine = FoldStableValues && field->is_stable() && cst.is_valid() && cst.as_boolean();

You already check `FoldStableValues` above. Same below.

src/hotspot/share/opto/type.cpp line 395:

> 393:     bool is_narrow_oop = (loadbt == T_NARROWOOP);
> 394:     return Type::make_from_constant(con, /*require_constant=*/true, stable_dimension, is_narrow_oop, /*is_autobox_cache=*/false);
> 395:       }

Indentation is off here.

test/hotspot/jtreg/compiler/c2/irTests/stable/StableRefArrayTest.java line 119:

> 117:     static int testPartialFold() {
> 118:         // Access should not be folded.
> 119:         // No barriers expected for plain fields.

Same comments as in `testNoFold`

test/hotspot/jtreg/compiler/c2/irTests/stable/StableRefPlainTest.java line 82:

> 80:     @IR(counts = { IRNode.LOAD, ">0" })
> 81:     @IR(applyIf = {"enable-valhalla", "false"}, failOn = { IRNode.MEMBAR })
> 82:     @IR(applyIf = {"enable-valhalla", "true"}, counts = { IRNode.MEMBAR, ">0" })

This needs a comment explaining why barriers are sometimes observed.

test/hotspot/jtreg/compiler/c2/irTests/stable/StableRefPlainTest.java line 85:

> 83:     static int testNoFold() {
> 84:         // Access should not be folded.
> 85:         // No barriers expected for plain fields.

This comment is outdated.

test/hotspot/jtreg/compiler/c2/irTests/stable/StableRefPlainTest.java line 116:

> 114:     @IR(applyIf = {"enable-valhalla", "true"}, counts = { IRNode.MEMBAR, ">0" })
> 115:     static void testMethodInit() {
> 116:         // Reference inits do not have membars.

Same comments as in `testNoFold`.

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

PR Review: https://git.openjdk.org/valhalla/pull/1826#pullrequestreview-3597631149
PR Review Comment: https://git.openjdk.org/valhalla/pull/1826#discussion_r2634410594
PR Review Comment: https://git.openjdk.org/valhalla/pull/1826#discussion_r2634261718
PR Review Comment: https://git.openjdk.org/valhalla/pull/1826#discussion_r2634263552
PR Review Comment: https://git.openjdk.org/valhalla/pull/1826#discussion_r2634400033
PR Review Comment: https://git.openjdk.org/valhalla/pull/1826#discussion_r2634403965
PR Review Comment: https://git.openjdk.org/valhalla/pull/1826#discussion_r2634408214
PR Review Comment: https://git.openjdk.org/valhalla/pull/1826#discussion_r2634300042
PR Review Comment: https://git.openjdk.org/valhalla/pull/1826#discussion_r2634293123
PR Review Comment: https://git.openjdk.org/valhalla/pull/1826#discussion_r2634274343
PR Review Comment: https://git.openjdk.org/valhalla/pull/1826#discussion_r2634274940
PR Review Comment: https://git.openjdk.org/valhalla/pull/1826#discussion_r2634276778