On Mon, 13 Jul 2026 18:47:18 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/gc/z/c2/zBarrierSetC2.cpp line 447: > >> 445: assert(UseCompactObjectHeaders, "should only happen with COH"); >> 446: assert((arrayOopDesc::base_offset_in_bytes(T_OBJECT) - offset) >> == BytesPerLong, "unexpected offset"); >> 447: length = phase->transform_later(new SubXNode(length, >> phase->longcon(1))); // Size is in longs > > Suggestion: > > length = phase->transform_later(new SubXNode(length, > phase->MakeConX(1))); // Size is in longs Addressing with [JDK-8388007](https://bugs.openjdk.org/browse/JDK-8388007). > src/hotspot/share/opto/addnode.cpp line 873: > >> 871: // own slice so we need to extract the field being accessed from >> 872: // the address computation >> 873: return tp->is_aryptr()->add_field_offset_and_offset(txoffset); > > Wouldn't it be better to fold the add_field_offset_and_offset() logic into > add_offset()? Having the caller decide then brings into question other call > sites that call add_offset() w/o checking first. I think that's a valid point. It's easy to call the wrong one in the future or simply forget that `add_field_offset_and_offset()` also exists now. Having one method removes that risk. We already have [JDK-8358079](https://bugs.openjdk.org/browse/JDK-8358079) to fix some known inconsistencies around the offset handling. I extended the scope of the RFE to also consider merging both methods as suggested here. > src/hotspot/share/opto/bytecodeInfo.cpp line 92: > >> 90: return true; // constructor >> 91: } >> 92: if ((caller_method->is_object_constructor() || >> caller_method->is_class_initializer()) && > > Isn't this JDK-8222787 logic obsolete? See comment above - addressing with [JDK-8388007](https://bugs.openjdk.org/browse/JDK-8388007). > src/hotspot/share/opto/callnode.cpp line 793: > >> 791: // create one projection per returned value. >> 792: assert(con <= TypeFunc::Parms+1 || InlineTypeReturnedAsFields, >> "only for multi value return"); >> 793: uint ideal_reg = range_cc->field_at(con)->ideal_reg(); > > There is only one caller of match() in matcher.cpp. We pass "mask" into all > match virtual functions just so CallNode can have it. But the caller always > uses return_values_mask() to compute the value, so why not just move that > here? > > Suggestion: > > uint ideal_reg = range_cc->field_at(con)->ideal_reg(); > const RegMask* mask = return_values_mask(range_cc); Right, this seems unnecessary. Let's move the mask computation here and remove the additional `mask` arg again. Addressing with [JDK-8388007](https://bugs.openjdk.org/browse/JDK-8388007). > src/hotspot/share/opto/callnode.cpp line 2056: > >> 2054: { >> 2055: assert(initializer != nullptr && >> 2056: (initializer->is_object_constructor() || >> initializer->is_class_initializer()), > > Isn't this JDK-8222787 logic obsolete? Yes, this looks like a left over that was missed to clean up with [JDK-8325660](https://bugs.openjdk.org/browse/JDK-8325660). Addressing this and the other two comments with [JDK-8388007](https://bugs.openjdk.org/browse/JDK-8388007). > src/hotspot/share/opto/loopnode.hpp line 90: > >> 88: MultiversionDelayedSlowLoop = 3<<17, >> 89: MultiversionFlagsMask = 3<<17, >> 90: FlatArrays = 1<<18}; > > It looks like this value overlaps with 2-bit Multiversion flag values above. > Suggestion: > > FlatArrays = 1<<19}; Good catch! That's indeed off. Fixing with [JDK-8388007](https://bugs.openjdk.org/browse/JDK-8388007). > src/hotspot/share/opto/matcher.cpp line 1096: > >> 1094: mask = return_values_mask(n->in(0)->as_Call()->tf()); >> 1095: } >> 1096: m = n->in(0)->as_Multi()->match(n->as_Proj(), this, >> mask); > > "mask" is only used by CallNode::match(). Let's move the call to > return_values_mask() there so we don't have to compute a mask that will be > ignored by other match() virtual functions. See comment above - addressing with [JDK-8388007](https://bugs.openjdk.org/browse/JDK-8388007). > src/hotspot/share/opto/mulnode.hpp line 220: > >> 218: virtual const Type* bottom_type() const { return >> TypeTuple::LONG_PAIR; } >> 219: >> 220: virtual Node* match(const ProjNode* proj, const Matcher* m, const >> RegMask* mask); > > Let's revert the addition of "mask" and compute it only where needed. See > comments in matcher.cpp and callnode.cpp. See comment above - addressing with [JDK-8388007](https://bugs.openjdk.org/browse/JDK-8388007). > src/hotspot/share/opto/parse1.cpp line 1108: > >> 1106: // exceptional returns, since they cannot publish normally. >> 1107: // >> 1108: if ((method()->is_object_constructor() || >> method()->is_class_initializer()) && > > Isn't this JDK-8222787 logic obsolete? See comment above - addressing with [JDK-8388007](https://bugs.openjdk.org/browse/JDK-8388007). > test/docs/ProblemList.txt line 59: > >> 57: >> ############################################################################# >> 58: >> 59: # Valhalla failures start here: > > Do we still need this line? Looks unneeded - removing with [JDK-8388007](https://bugs.openjdk.org/browse/JDK-8388007). ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/31122#discussion_r3578582007 PR Review Comment: https://git.openjdk.org/jdk/pull/31122#discussion_r3578583699 PR Review Comment: https://git.openjdk.org/jdk/pull/31122#discussion_r3578587275 PR Review Comment: https://git.openjdk.org/jdk/pull/31122#discussion_r3578588886 PR Review Comment: https://git.openjdk.org/jdk/pull/31122#discussion_r3578585889 PR Review Comment: https://git.openjdk.org/jdk/pull/31122#discussion_r3578590787 PR Review Comment: https://git.openjdk.org/jdk/pull/31122#discussion_r3578589340 PR Review Comment: https://git.openjdk.org/jdk/pull/31122#discussion_r3578589927 PR Review Comment: https://git.openjdk.org/jdk/pull/31122#discussion_r3578586946 PR Review Comment: https://git.openjdk.org/jdk/pull/31122#discussion_r3578587767
