On Thu, 2 Jul 2026 13:57:08 GMT, Roland Westrelin <[email protected]> wrote:
>> This addresses Andrew's comment: >> >> https://github.com/openjdk/jdk/pull/31122#discussion_r3499128872 >> >> One of the 2 stubs are no longer used. I removed references to it. >> >> --------- >> - [x] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > Roland Westrelin has updated the pull request incrementally with one > additional commit since the last revision: > > more This is all good. There is still one small tweak needed. The list of stubs generated into the cache depends on the setting of `InlineTypeReturnedAsFields`. So, this flag setting needs to be recorded in the AOT cache when it is dumped and checked for consistency when the cache is loaded. That can be done by adding an entry to macro `AOTCODECACHE_CONFIGS_GENERIC_DO` in `src/hotspot/shared/code/aotCodecache.hpp` #define AOTCODECACHE_CONFIGS_GENERIC_DO(do_var, do_fun) \ do_var(int, AllocateInstancePrefetchLines) /* stubs and nmethods */ \ . . . do_var(boolean, InlineTypeReturnedAsFields) /* stubs */ \ . . . Also, just checking: flag `InlineTypeReturnedAsFields` should not currently be set on any arch other than x86 or aarch64 because only those two generate the blob. Is there startup argument checking logic to detect an invalid setting? If not then can that lead to a crash on other arches should someone set it on the command line? src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp line 3184: > 3182: } > 3183: > 3184: // Call here from the interpreter or compiled code to either load This comment need to be updated to reflect the fact that the load stub no longer exists. src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp line 3188: > 3186: // returned to registers or to store returned values to a newly > 3187: // allocated inline type instance. > 3188: RuntimeStub* SharedRuntime::generate_return_value_stub(address > destination, bool has_res) { This only gets called with `has_res = true` so the parameter should be dropped and the code below for that case hard-wired. src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp line 3303: > 3301: __ cbnz(rscratch1, pending); > 3302: > 3303: if (has_res) { The if here is not needed. The if body can simply be inlined. src/hotspot/cpu/x86/sharedRuntime_x86_64.cpp line 3880: > 3878: } > 3879: > 3880: // Call here from the interpreter or compiled code to either load Same as with aarch64: I think this comment needs to be updated to omit reference to the load operation which is no longer used. src/hotspot/cpu/x86/sharedRuntime_x86_64.cpp line 3888: > 3886: PRAGMA_DIAG_PUSH > 3887: PRAGMA_NONNULL_IGNORED > 3888: RuntimeStub* SharedRuntime::generate_return_value_stub(address > destination, bool has_res) { Same as with aarch64: This only gets called with `has_res = true` so the parameter should be dropped and the code below for that case hard-wired. src/hotspot/cpu/x86/sharedRuntime_x86_64.cpp line 4014: > 4012: __ jcc(Assembler::notEqual, pending); > 4013: > 4014: if (has_res) { Same as with aarch64: The `if` here is not needed. The `if` body can simply be inlined. ------------- Changes requested by adinn (no project role). PR Review: https://git.openjdk.org/valhalla/pull/2616#pullrequestreview-4618688409 PR Review Comment: https://git.openjdk.org/valhalla/pull/2616#discussion_r3514081309 PR Review Comment: https://git.openjdk.org/valhalla/pull/2616#discussion_r3514062928 PR Review Comment: https://git.openjdk.org/valhalla/pull/2616#discussion_r3514066481 PR Review Comment: https://git.openjdk.org/valhalla/pull/2616#discussion_r3514009269 PR Review Comment: https://git.openjdk.org/valhalla/pull/2616#discussion_r3514016931 PR Review Comment: https://git.openjdk.org/valhalla/pull/2616#discussion_r3514024901
