On Thu, 18 Dec 2025 12:01:36 GMT, Marc Chevalier <[email protected]> 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
