On Mon, 11 May 2026 14:19:50 GMT, David Simms <[email protected]> wrote:
> This is a "*sub-review pull request*" for the first > [preview](https://openjdk.org/jeps/12) of [JEP 401: Value Classes and > Objects](https://openjdk.org/jeps/401), specifically > [JDK-8317278](https://bugs.openjdk.org/browse/JDK-8317278): JVM > implementation of value classes and objects. > >> [!NOTE] >> This pull request and the other sub-review pull requests listed below are >> based on the "*master pull request*" >> (https://github.com/openjdk/jdk/pull/31120). It contains the same full set >> of code changes as the "*master pull request*" to preserve the full >> implementation context; the language compiler, JVM, and standard library >> changes are intertwined. This separate pull requests exist only to subdivide >> the review and related discussion by area. > > Other areas for review: > > - [JDK-8317277](https://bugs.openjdk.org/browse/JDK-8317277): Java language > implementation of value classes and objects > - https://github.com/openjdk/jdk/pull/31121 > - [JDK-8317279](https://bugs.openjdk.org/browse/JDK-8317279): Standard > library implementation of value classes and objects > - https://github.com/openjdk/jdk/pull/31123 > > Code changes resulting from the review process should be made in > [`valhalla/lworld`](https://github.com/openjdk/valhalla/). > > `valhalla/lworld` is currently updated from `jdk/master` whenever a weekly > [`jdk` tag](https://github.com/openjdk/jdk/tags) is created. At that time, > code changes from `valhalla/lworld` will be propagated to the master pull > request and to all sub-review pull requests, including this one. > > Ultimately, review sign-off will be recorded on the "*master pull request*", > and this pull request will be closed without integration. > > This pull request has a large code surface area and often conflicts with > `jdk/master` on a daily basis. Refer to > [`valhalla/lworld`](https://github.com/openjdk/valhalla/) for the latest > state of the project code, keeping in mind that it may lag several days > behind `jdk/master`. Both repositories may be needed as references during > review. > > --------- > - [x] I confirm that I make this contribution in accordance with the [OpenJDK > Interim AI Policy](https://openjdk.org/legal/ai). src/hotspot/cpu/aarch64/aarch64.ad line 14185: > 14183: < (uint64_t)(BlockZeroingLowLimit >> LogBytesPerWord) > 14184: && !((ClearArrayNode*)n)->word_copy_only()); > 14185: match(Set dummy (ClearArray cnt base)); I don't see how this can match if the new shape is "ClearArray (Binary cnt base) val". src/hotspot/cpu/aarch64/gc/g1/g1_aarch64.ad line 98: > 96: > 97: // Extract the narrow oop field value > 98: __ ubfm($tmp1$$Register, $src$$Register, 0, 31); Using `uxtw` or `movw` here might be more clear if the value is always the low word and not an arbitrary bitmask. src/hotspot/cpu/s390/continuationHelper_s390.inline.hpp line 2: > 1: /* > 2: * Copyright (c) 2022, 2026, Oracle and/or its affiliates. All rights > reserved. I see no other changes in this file. src/hotspot/cpu/x86/x86.ad line 2633: > 2631: if (nops_cnt > 0) { > 2632: __ nop(nops_cnt); > 2633: } Suggestion: There is no patch_verified_entry() anymore. src/hotspot/share/c1/c1_LIRGenerator.cpp line 1550: > 1548: static LIR_Opr null_marker_mask(BasicType bt, int nm_offset) { > 1549: assert(nm_offset >= 0, "field does not have null marker"); > 1550: jlong null_marker = 1ULL << (nm_offset << LogBitsPerByte); Doesn't this give the wrong value for big-endian? src/hotspot/share/oops/instanceKlass.hpp line 152: > 150: InlineLayoutInfo(): _klass(nullptr), _kind(LayoutKind::UNKNOWN), > _null_marker_offset(-1) {} > 151: InlineLayoutInfo(InlineKlass* ik, LayoutKind kind, int size, int > nm_offset): > 152: _klass(ik), _kind(kind), _null_marker_offset(nm_offset) {} This ctor seems to be unused. src/hotspot/share/opto/inlinetypenode.cpp line 2228: > 2226: // in size and would then be written by a "normal" oop store. If the > payload contains oops, its size is always > 2227: // 64-bit because the next smaller (power-of-two) size would be > 32-bit which could only hold one narrow oop that > 2228: // would then be written by a normal narrow oop store. These > properties are asserted in 'convert_to_payload'. I was a little surprised that this is hard-coded to 64-bit, when x64 and aarch64 can do 128-bit atomic reads and writes. But maybe the problem is that GPRs are 64 bit, so a 128-bit memory op would require a register pair or vector register? src/hotspot/share/opto/inlinetypenode.cpp line 2331: > 2329: } > 2330: > 2331: Node* shift_val = igvn.intcon(offset << LogBitsPerByte); The way values are packed and then read or written in the x86 and aarch64 back-end seems to imply little-endian. For big-endian, it seems like we need to pack in different order, or force the back-end to byte-swap. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/31122#discussion_r3257032459 PR Review Comment: https://git.openjdk.org/jdk/pull/31122#discussion_r3231825252 PR Review Comment: https://git.openjdk.org/jdk/pull/31122#discussion_r3271518882 PR Review Comment: https://git.openjdk.org/jdk/pull/31122#discussion_r3240119982 PR Review Comment: https://git.openjdk.org/jdk/pull/31122#discussion_r3232108282 PR Review Comment: https://git.openjdk.org/jdk/pull/31122#discussion_r3232317653 PR Review Comment: https://git.openjdk.org/jdk/pull/31122#discussion_r3231433009 PR Review Comment: https://git.openjdk.org/jdk/pull/31122#discussion_r3231848607
