On Mon, 26 Jan 2026 14:01:08 GMT, Quan Anh Mai <[email protected]> wrote:
> Hi, > > This PR implements the support of `NULLABLE_NON_ATOMIC_FLAT` layout in the > JITs. There is nothing to do in C2. In C1, I need to implement > loading/storing nullable value of a non-atomic field. > > The test `TestValueClasses` is failing with `-XX:-TieredCompilation`, which I > suspect is due to the substitutability test handling padding bytes > incorrectly. > > Please kindly review what there are for now, thanks a lot. Thanks for jumping right on this, Quan Anh! The fix looks good to me, I just added a few minor comments. > The test TestValueClasses is failing with -XX:-TieredCompilation, which I > suspect is due to the substitutability test handling padding bytes > incorrectly. I thought this got fixed - do we have a tracking bug for this? src/hotspot/share/c1/c1_GraphBuilder.cpp line 2067: > 2065: Value nm_offset = append(new Constant(new > LongConstant(offset + inline_klass->null_marker_offset_in_payload()))); > 2066: Value nm = append(new UnsafeGet(T_BOOLEAN, obj, > nm_offset, false)); > 2067: result = append(new IfOp(nm, Instruction::neq, > int_zero, new_instance, object_null, state_before, false)); Would be nice to replace this code in the LIRGenerator by HIR instructions as well: https://github.com/openjdk/valhalla/blob/5c3e588a00ba28b2f9d095f5cbc86fa63958c21a/src/hotspot/share/c1/c1_LIRGenerator.cpp#L2124-L2127 But IIRC, that was non-trivial and is definitely out-of-scope of this PR. src/hotspot/share/ci/ciInlineKlass.cpp line 159: > 157: } > 158: > 159: // All fields of this object is zero even if they can be null-free. As a > result, this object should Suggestion: // All fields of this object are zero even if they are null-free. As a result, this object should src/hotspot/share/opto/parse3.cpp line 285: > 283: inc_sp(1); > 284: bool is_immutable = field->is_final() && field->is_strict(); > 285: bool atomic = vk->must_be_atomic() || !field->is_null_free(); Can `must_be_atomic` be removed now? src/hotspot/share/runtime/globals.hpp line 842: > 840: > \ > 841: product(bool, UseNullableNonAtomicValueFlattening, true, > \ > 842: "Allow the JVM to flatten some strict final non-static > fields") \ Suggestion: "Allow the JVM to flatten some strict final non-static fields") \ ------------- Marked as reviewed by thartmann (Committer). PR Review: https://git.openjdk.org/valhalla/pull/1970#pullrequestreview-3709286745 PR Review Comment: https://git.openjdk.org/valhalla/pull/1970#discussion_r2730520300 PR Review Comment: https://git.openjdk.org/valhalla/pull/1970#discussion_r2730528922 PR Review Comment: https://git.openjdk.org/valhalla/pull/1970#discussion_r2730533413 PR Review Comment: https://git.openjdk.org/valhalla/pull/1970#discussion_r2730415298
