On Mon, 20 Apr 2026 12:31:27 GMT, Tobias Hartmann <[email protected]> wrote:
> This patch addresses various follow-up TODOs from > [JDK-8278390](https://bugs.openjdk.org/browse/JDK-8278390). > > I added PR comments to explain the changes in more detail. > > Thanks, > Tobias > > --------- > - [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 2206: > 2204: __ ic_check(1); > 2205: } else { > 2206: // TODO 8284443 Avoid creation of temporary frame I don't think we can easily avoid this, unfortunately. But I added a comment that explains why it's needed. See [JDK-8293994](https://bugs.openjdk.org/browse/JDK-8293994) for background. src/hotspot/cpu/aarch64/c1_MacroAssembler_aarch64.cpp line 317: > 315: bs->nmethod_entry_barrier(this, nullptr /* slow_path */, nullptr /* > continuation */, nullptr /* guard */); > 316: > 317: // FIXME -- call runtime only if we cannot in-line allocate all the > incoming inline type args. Not something we want to implement at this point. src/hotspot/cpu/aarch64/frame_aarch64.inline.hpp line 466: > 464: if (nm != nullptr && nm->is_compiled_by_c1() && > nm->method()->has_scalarized_args() && > 465: pc() < nm->verified_inline_entry_point()) { > 466: // TODO 8284443 Can't we do that by not passing > 'dont_gc_arguments' in case 'StubId::c1_buffer_inline_args_id' in > 'Runtime1::generate_code_for'? No because we need this for the nmethod at method entry only, not for the stub itself. src/hotspot/cpu/aarch64/interp_masm_aarch64.cpp line 749: > 747: bind(skip_stress); > 748: } > 749: #endif The problem is that `MethodFlags::has_scalarized_return_flag` is not accurate because it's set at method link time when the return type might not be loaded yet. After this patch, it's also not used anywhere else but for this stress path, so I decided to simply remove it. The same stress path exists in C1 and C2 which is sufficient for stress testing. src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp line 7468: > 7466: } > 7467: > 7468: // The GC barrier expanded by store_heap_oop below may call into the See [JDK-8382417](https://bugs.openjdk.org/browse/JDK-8382417). src/hotspot/cpu/x86/c1_MacroAssembler_x86.cpp line 309: > 307: bs->nmethod_entry_barrier(this, nullptr /* slow_path */, nullptr /* > continuation */); > 308: > 309: // FIXME -- call runtime only if we cannot in-line allocate all the > incoming inline type args. Not something we want to implement at this point. src/hotspot/cpu/x86/frame_x86.inline.hpp line 443: > 441: if (nm != nullptr && nm->is_compiled_by_c1() && > nm->method()->has_scalarized_args() && > 442: pc() < nm->verified_inline_entry_point()) { > 443: // TODO 8284443 Can't we do that by not passing > 'dont_gc_arguments' in case 'StubId::c1_buffer_inline_args_id' in > 'Runtime1::generate_code_for'? No because we need this for the nmethod at method entry only, not for the stub itself. src/hotspot/cpu/x86/interp_masm_x86.cpp line 1100: > 1098: orptr(rax, 1); > 1099: bind(skip_stress); > 1100: } The problem is that MethodFlags::has_scalarized_return_flag is not accurate because it's set at method link time when the return type might not be loaded yet. After this patch, it's also not used anywhere else but for this stress path, so I decided to simply remove it. The same stress path exists in C1 and C2 which is sufficient for stress testing. src/hotspot/cpu/x86/macroAssembler_x86.cpp line 6464: > 6462: } > 6463: from_index--; > 6464: assert(progress || (start_offset == offset()), "should not emit > code"); See [JDK-8382417](https://bugs.openjdk.org/browse/JDK-8382417). src/hotspot/cpu/x86/x86.ad line 2608: > 2606: __ ic_check(1); > 2607: } else { > 2608: // TODO 8284443 Avoid creation of temporary frame I don't think we can easily avoid this, unfortunately. But I added a comment that explains why it's needed. See [JDK-8293994](https://bugs.openjdk.org/browse/JDK-8293994) for background. src/hotspot/share/asm/macroAssembler_common.cpp line 93: > 91: int MacroAssembler::unpack_inline_args(Compile* C, bool receiver_only) { > 92: assert(C->has_scalarized_args(), "inline type argument scalarization is > disabled"); > 93: ciMethod* method = C->method(); Since this is called from the compiler threads, all accesses should go through the CI. src/hotspot/share/c1/c1_Compilation.cpp line 603: > 601: ResetNoHandleMark rnhm; > 602: // TODO 8284443 Should only be computed once > 603: _compiled_entry_signature.compute_calling_conventions(false); I move this to where it's needed but caching doesn't seem feasible. src/hotspot/share/oops/inlineKlass.cpp line 197: > 195: int offset = base_off + fs.offset() - (base_off > 0 ? > payload_offset() : 0); > 196: InstanceKlass* field_holder = fs.field_descriptor().field_holder(); > 197: // TODO 8284443 Use different heuristic to decide what should be > scalarized in the calling convention I think the heuristic is good as-is. src/hotspot/share/oops/method.cpp line 1340: > 1338: } > 1339: if (InlineTypeReturnedAsFields && returns_inline_type() && > !has_scalarized_return()) { > 1340: set_has_scalarized_return(); Removed because it's not accurate. See comments above. src/hotspot/share/opto/callGenerator.cpp line 1078: > 1076: Node* n = slow_map->in(i); > 1077: const Type* t = gvn.type(m)->meet_speculative(gvn.type(n)); > 1078: // TODO 8284443 still needed? Turns out the entire block is not needed anymore. src/hotspot/share/opto/chaitin.cpp line 2467: > 2465: reg = OptoReg::add(reg, -1); > 2466: > 2467: // Special fixed slots Added improved printing of the special fixed stack slots. "stack increment" and "null marker" is Valhalla specific. "original deopt pc" is not. src/hotspot/share/opto/graphKit.cpp line 2033: > 2031: Node* arg = argument(arg_idx); > 2032: const Type* t = domain->field_at(i); > 2033: // TODO 8284443 A static call to a mismatched method should still > be scalarized Not worth the complexity because this would also need changes in `TypeFunc::make` where the "is virtual call" information is messy to propagate. Let's leave it for now. src/hotspot/share/opto/inlinetypenode.cpp line 1148: > 1146: // AllocateNode::Ideal_allocation(oop, &gvn) != nullptr || > vt->as_InlineType()->is_loaded(&gvn) == oop, "inline type should be loaded"); > 1147: } > 1148: assert(vt->is_allocated(&gvn), "inline type should be allocated"); This assert is just way too brittle. I fixed it multiple times before and I don't think it's useful. Let's just remove it. src/hotspot/share/opto/inlinetypenode.cpp line 1391: > 1389: } else { > 1390: n->init_req(base_input++, get_oop()); > 1391: } Filed [JDK-8382527](https://bugs.openjdk.org/browse/JDK-8382527) as a follow-up on this. src/hotspot/share/opto/inlinetypenode.cpp line 1425: > 1423: } > 1424: } > 1425: I removed the requirement to return this arg, see changes to `src/hotspot/share/opto/type.cpp`. src/hotspot/share/opto/type.cpp line 7348: > 7346: ciSignature* sig = method->signature(); > 7347: bool has_scalar_ret = !method->is_native() && > sig->return_type()->is_inlinetype() && > sig->return_type()->as_inline_klass()->can_be_returned_as_fields(); > 7348: // Don't cache on scalarized return because the range depends on > 'is_call' We need to distinguish between the return signature of a method at calls vs. at compilation of that method because at calls we return an additional `InlineTypeNode::NullMarker` field. ------------- PR Review Comment: https://git.openjdk.org/valhalla/pull/2339#discussion_r3110636481 PR Review Comment: https://git.openjdk.org/valhalla/pull/2339#discussion_r3110649711 PR Review Comment: https://git.openjdk.org/valhalla/pull/2339#discussion_r3110652112 PR Review Comment: https://git.openjdk.org/valhalla/pull/2339#discussion_r3110674871 PR Review Comment: https://git.openjdk.org/valhalla/pull/2339#discussion_r3111176681 PR Review Comment: https://git.openjdk.org/valhalla/pull/2339#discussion_r3110678899 PR Review Comment: https://git.openjdk.org/valhalla/pull/2339#discussion_r3110679921 PR Review Comment: https://git.openjdk.org/valhalla/pull/2339#discussion_r3110683116 PR Review Comment: https://git.openjdk.org/valhalla/pull/2339#discussion_r3111177949 PR Review Comment: https://git.openjdk.org/valhalla/pull/2339#discussion_r3110688894 PR Review Comment: https://git.openjdk.org/valhalla/pull/2339#discussion_r3110692222 PR Review Comment: https://git.openjdk.org/valhalla/pull/2339#discussion_r3110697446 PR Review Comment: https://git.openjdk.org/valhalla/pull/2339#discussion_r3110700753 PR Review Comment: https://git.openjdk.org/valhalla/pull/2339#discussion_r3110703116 PR Review Comment: https://git.openjdk.org/valhalla/pull/2339#discussion_r3110705220 PR Review Comment: https://git.openjdk.org/valhalla/pull/2339#discussion_r3110711373 PR Review Comment: https://git.openjdk.org/valhalla/pull/2339#discussion_r3110912740 PR Review Comment: https://git.openjdk.org/valhalla/pull/2339#discussion_r3110796944 PR Review Comment: https://git.openjdk.org/valhalla/pull/2339#discussion_r3110799543 PR Review Comment: https://git.openjdk.org/valhalla/pull/2339#discussion_r3110856373 PR Review Comment: https://git.openjdk.org/valhalla/pull/2339#discussion_r3110935290
