On Thu, 9 Jul 2026 22:50:47 GMT, Dean Long <[email protected]> wrote:
>> David Simms has updated the pull request incrementally with 11 additional >> commits since the last revision: >> >> - Merge branch '8317277' into jep401_sub_review_8317278 >> - Merge remote-tracking branch 'valhalla/lworld' into 8317277 >> - 8387703: [lworld] address batch of comments in mainline PR >> >> Co-authored-by: Tobias Hartmann <[email protected]> >> Reviewed-by: thartmann, dlong >> - 8387661: [lworld] move generate_return_value_stub() to SharedRuntime >> >> Reviewed-by: adinn, thartmann >> - 8387405: [lworld] is_always_flat_in_array should be always false >> >> Reviewed-by: thartmann >> - 8387686: [lworld] Update the JNI specification version for value types >> >> Reviewed-by: alanb, dsimms >> - 8387726: [lworld] JVMTI/JDWP/JDI docs cleanup >> >> Reviewed-by: sspitsyn, cjplummer >> - 8387673: [lworld] C2: clone() intrinsic does not throw >> CloneNotSupportedException for value classes not implementing Cloneable >> >> Reviewed-by: thartmann >> - 8387709: [lworld] Split Valhalla tests from tier1_compiler_3 to avoid >> task timeouts >> >> Reviewed-by: chagedorn >> - 8387350: [lworld] ObjectOutputStream.writeObject() doesn't throw >> InvalidClassException for Serializable instance with strict init fields >> >> Reviewed-by: alanb, jpai, thartmann >> - ... and 1 more: https://git.openjdk.org/jdk/compare/f1f6d35d...1b6f7c18 > > src/hotspot/share/ci/ciClassList.hpp line 120: > >> 118: friend class ciType; \ >> 119: friend class ciReturnAddress; \ >> 120: friend class ciWrapper; \ > > Suggestion: > > friend class ciWrapper; \ Addressing with [JDK-8388007](https://bugs.openjdk.org/browse/JDK-8388007). > src/hotspot/share/ci/ciMethod.cpp line 1051: > >> 1049: // Note: We can't test is_static, because that would >> 1050: // require the method to be loaded. Sometimes it isn't. >> 1051: } > > This looks obsolete, and it no longer matches Method::is_object_constructor(). Yes, this should also have been reverted with as part of [JDK-8247569](https://bugs.openjdk.org/browse/JDK-8247569). Addressing with [JDK-8388007](https://bugs.openjdk.org/browse/JDK-8388007). > src/hotspot/share/ci/ciSignature.hpp line 60: > >> 58: >> 59: ciType* return_type() const { return _return_type; } >> 60: ciType* type_at(int index) const { return >> _types.at(index)->unwrap(); } > > AFAICT, we only use type wrappers in ciTypeFlow.cpp, and we never add them to > signatures. It might be better to assert t == t->unwrap() here before > returning the result. We could also put a similar assert in the ciSignature > ctor. Addressing with [JDK-8388007](https://bugs.openjdk.org/browse/JDK-8388007). > src/hotspot/share/ci/ciType.hpp line 130: > >> 128: EarlyLarval = NullFree << 1, >> 129: }; >> 130: int _properties; > > A wrapper class adds complexity for only 2 bits of additional information. > Also wrapped types don't behave exactly like unwrapped types, so the caller > needs to know if it is wrapped or not. I suggest an RFE that investigates > moving the extra property bits into ciType. I filed [JDK-8388070](https://bugs.openjdk.org/browse/JDK-8388070) to find a better representation. I'm not sure, though, about moving these bits directly into `ciType` since they describe individual type flow values that may differ. But this can be further investigated in [JDK-8388070](https://bugs.openjdk.org/browse/JDK-8388070). > src/hotspot/share/ci/ciTypeFlow.cpp line 581: > >> 579: // ------------------------------------------------------------------ >> 580: // ciTypeFlow::StateVector::do_aload >> 581: void ciTypeFlow::StateVector::do_aload(ciBytecodeStream* str) { > > This is only called because of Bytecodes::_aaload, so if there was a > historical reason for the rename, I don't see it now. It's been there since the import from MVT. Back there, it was shared by `_vaload` and `_aaload`. That might have been the reason. Anyway, these v-bytecodes are long gone, so we should revert back to `aaload`. Addressing with [JDK-8388007](https://bugs.openjdk.org/browse/JDK-8388007). > src/hotspot/share/ci/ciTypeFlow.cpp line 583: > >> 581: void ciTypeFlow::StateVector::do_aload(ciBytecodeStream* str) { >> 582: pop_int(); >> 583: ciArrayKlass* array_klass = pop_objOrFlatArray(); > > Obsolete. Consider going back to old name and narrower type. > Suggestion: > > ciObjArrayKlass* array_klass = pop_objArray(); Addressing with [JDK-8388007](https://bugs.openjdk.org/browse/JDK-8388007). > src/hotspot/share/ci/ciTypeFlow.cpp line 964: > >> 962: pop_object(); >> 963: pop_int(); >> 964: pop_objOrFlatArray(); > > Now that a ciFlatArrayKlass "is a" ciObjArrayKlass, consider going back to > the old name pop_objArray(). Addressing with [JDK-8388007](https://bugs.openjdk.org/browse/JDK-8388007). > src/hotspot/share/ci/ciTypeFlow.cpp line 986: > >> 984: trap(str, element_klass, str->get_klass_index()); >> 985: } else { >> 986: push_object(ciArrayKlass::make(element_klass)); > > Suggestion: > > push_object(ciObjArrayKlass::make(element_klass)); > > Can we revert this in JDK-8388007? Addressing with [JDK-8388007](https://bugs.openjdk.org/browse/JDK-8388007). > src/hotspot/share/ci/ciTypeFlow.cpp line 3228: > >> 3226: >> 3227: ciType* ciTypeFlow::mark_as_early_larval(ciType* type) { >> 3228: // Wrap the type to carry the information that it is null-free > > Suggestion: > > // Wrap the type to carry the information that it is "early larval" Seems like a copy-paste error. Addressing with [JDK-8388007](https://bugs.openjdk.org/browse/JDK-8388007). > src/hotspot/share/ci/ciTypeFlow.hpp line 345: > >> 343: assert(array->is_obj_array_klass(), "must be an object array >> type"); >> 344: return array->as_array_klass(); >> 345: } > > Obsolete. Consider going back to `ciObjArrayKlass* pop_objArray()`. If we > think the comment needs to mention ciFlatArrayKlass, then ciRefArrayKlass > should also be mentioned, but I think the original comment from JDK mainline > is fine. Right, let's change the name back and restore the original comment. Addressing with [JDK-8388007](https://bugs.openjdk.org/browse/JDK-8388007). > src/hotspot/share/ci/ciTypeFlow.hpp line 358: > >> 356: >> 357: // Helper convenience routines. >> 358: void do_aload(ciBytecodeStream* str); > > Why the rename? The old name seem more accurate. See answer below. Addressing with [JDK-8388007](https://bugs.openjdk.org/browse/JDK-8388007). > src/hotspot/share/code/scopeDesc.hpp line 111: > >> 109: // any of the scopes at >> compiled pc. >> 110: bool _arg_escape; // Compiled Java call in >> youngest scope passes ArgEscape >> 111: > > Restore blank line. Addressing with [JDK-8388007](https://bugs.openjdk.org/browse/JDK-8388007). ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/31122#discussion_r3558270852 PR Review Comment: https://git.openjdk.org/jdk/pull/31122#discussion_r3558270201 PR Review Comment: https://git.openjdk.org/jdk/pull/31122#discussion_r3558271072 PR Review Comment: https://git.openjdk.org/jdk/pull/31122#discussion_r3558271420 PR Review Comment: https://git.openjdk.org/jdk/pull/31122#discussion_r3558276345 PR Review Comment: https://git.openjdk.org/jdk/pull/31122#discussion_r3558273956 PR Review Comment: https://git.openjdk.org/jdk/pull/31122#discussion_r3558273643 PR Review Comment: https://git.openjdk.org/jdk/pull/31122#discussion_r3558273065 PR Review Comment: https://git.openjdk.org/jdk/pull/31122#discussion_r3558272735 PR Review Comment: https://git.openjdk.org/jdk/pull/31122#discussion_r3558274498 PR Review Comment: https://git.openjdk.org/jdk/pull/31122#discussion_r3558275108 PR Review Comment: https://git.openjdk.org/jdk/pull/31122#discussion_r3558276560
