On Fri, 13 Mar 2026 16:14:27 GMT, Paul Hübner <[email protected]> wrote:
> Hi all,
>
> This PR cleans up Valhalla flags. To summarize (for the exhaustive changes,
> please refer to the diff):
> - All notions of "inline" are replaced with "value" in product flags.
> - This is to be consistent with the language, but also other VM flags which
> already use "value".
> - The granular flattening control flags have received consistent names.
> - The granular flattening control flags have been diagnosticized.
> - Setting granular flattening control flags will give a VM warning is
> flattening is not enabled.
> - Flat array printing has been debugized.
> - Descriptions are updated.
> - Updated all tests to use the new flags.
>
> Testing: tiers 1-3.
Thank you! The less usage of the word "inline" to describe values, the better
IMO. I've skimmed through the patch and I think it looks good. I've added a
couple of nits that you might want to consider.
src/hotspot/share/opto/parse2.cpp line 130:
> 128: Node* inline_type = load_from_unknown_flat_array(array,
> array_index, element_ptr);
> 129: bool is_null_free = array_type->is_null_free()
> 130: || (!UseNullableAtomicValueFlattening &&
> !UseNullableNonAtomicValueFlattening);
I know that some RT code tend to put the operators at the previous line and I
see a precedent in this file:
assert(UseArrayFlattening && is_reference_type(bt) &&
element_ptr->can_be_inline_type() &&
(!element_ptr->is_inlinetypeptr() ||
element_ptr->inline_klass()->maybe_flat_in_array()), "array can't be flat");
so maybe:
Suggestion:
bool is_null_free = array_type->is_null_free() ||
(!UseNullableAtomicValueFlattening &&
!UseNullableNonAtomicValueFlattening);
src/hotspot/share/runtime/arguments.cpp line 3884:
> 3882: #define WARN_IF_NOT_DEFAULT_FLAG(flag)
> \
> 3883: if (!FLAG_IS_DEFAULT(flag)) {
> \
> 3884: warning("Preview-specific flag \"%s\" has no effect when
> --enable-preview is not specified.", #flag); \
Is this done because we don't want to mention Valhalla to the users?
src/hotspot/share/runtime/arguments.cpp line 3942:
> 3940: && !UseNullableAtomicValueFlattening
> 3941: && !UseNullFreeAtomicValueFlattening
> 3942: && !UseNullableNonAtomicValueFlattening) {
Suggestion:
if (!UseNullFreeNonAtomicValueFlattening &&
!UseNullableAtomicValueFlattening &&
!UseNullFreeAtomicValueFlattening &&
!UseNullableNonAtomicValueFlattening) {
test/hotspot/jtreg/compiler/valhalla/inlinetypes/InlineTypes.java line 41:
> 39: "--add-exports",
> "java.base/jdk.internal.vm.annotation=ALL-UNNAMED",
> 40: "--add-exports",
> "java.base/jdk.internal.misc=ALL-UNNAMED",
> 41: "-XX:+UnlockDiagnosticVMOptions",
Unrelated to this PR, but I saw this flag:
"-XX:+IgnoreUnrecognizedVMOptions",
I would consider removing from all the tests as a follow-up PR. We've been
bitten by that flag so many times in our testing that we shouldn't use it at
all, unless there's a written motivation why it is needed. My 2c.
-------------
PR Review:
https://git.openjdk.org/valhalla/pull/2226#pullrequestreview-3951793887
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2226#discussion_r2938500801
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2226#discussion_r2938503854
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2226#discussion_r2938515350
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2226#discussion_r2938525196
