On Fri, 24 Mar 2023 15:13:40 GMT, Matias Saavedra Silva <matsa...@openjdk.org> wrote:
>> The current structure used to store the resolution information for >> invokedynamic, ConstantPoolCacheEntry, is difficult to interpret due to its >> ambigious fields f1 and f2. This structure can hold information for fields, >> methods, and invokedynamics and each of its fields can hold different types >> of values depending on the entry. >> >> This enhancement proposes a new structure to exclusively contain >> invokedynamic information in a manner that is easy to interpret and easy to >> extend. Resolved invokedynamic entries will be stored in an array in the >> constant pool cache and the operand of the invokedynamic bytecode will be >> rewritten to be the index into this array. >> >> Any areas that previously accessed invokedynamic data from >> ConstantPoolCacheEntry will be replaced with accesses to this new array and >> structure. Verified with tier1-9 tests. >> >> The PPC was provided by @reinrich and the RISCV port was provided by >> @DingliZhang and @zifeihan. >> >> This change supports the following platforms: x86, aarch64, PPC, and RISCV > > Matias Saavedra Silva has updated the pull request incrementally with one > additional commit since the last revision: > > Improved interpreter comments aarch64 Changes requested by fyang (Reviewer). src/hotspot/cpu/aarch64/templateTable_aarch64.cpp line 2344: > 2342: // Compare the method to zero > 2343: __ tst(method, method); > 2344: __ br(Assembler::NE, resolved); Consider use 'cbnz' instruction here, like: __ cbnz(method, resolved) src/hotspot/cpu/aarch64/templateTable_aarch64.cpp line 2360: > 2358: #ifdef ASSERT > 2359: __ tst(method, method); > 2360: __ br(Assembler::NE, resolved); Same here, like: __ cbnz(method, resolved); src/hotspot/cpu/riscv/templateInterpreterGenerator_riscv.cpp line 452: > 450: } else { > 451: // Pop N words from the stack > 452: __ get_cache_and_index_at_bcp(x11, x12, 1, index_size); Better to use 'cache' and 'index' in this branch instead of 'x11' and 'x12'. src/hotspot/cpu/riscv/templateTable_riscv.cpp line 2218: > 2216: } > 2217: > 2218: void TemplateTable::load_invokedynamic_entry(Register method) { Please also add a comment for this function, like aarch64. src/hotspot/cpu/riscv/templateTable_riscv.cpp line 2236: > 2234: // Compare the method to zero > 2235: __ andr(t0, method, method); > 2236: __ bnez(t0, resolved); I think a more simpler "__ bnez(method, resolved)" will do. src/hotspot/cpu/riscv/templateTable_riscv.cpp line 2243: > 2241: address entry = CAST_FROM_FN_PTR(address, > InterpreterRuntime::resolve_from_cache); > 2242: __ mv(method, code); // this is essentially Bytecodes::_invokedynamic > 2243: __ call_VM(noreg, entry, method); // Example uses temp = rbx. In this > case rbx is method Remove the code comment here as we don't have rbx for riscv. src/hotspot/cpu/riscv/templateTable_riscv.cpp line 2252: > 2250: #ifdef ASSERT > 2251: __ andr(t0, method, method); > 2252: __ bnez(t0, resolved); Consider "__ bnez(method, resolved)". ------------- PR Review: https://git.openjdk.org/jdk/pull/12778#pullrequestreview-1357775328 PR Review Comment: https://git.openjdk.org/jdk/pull/12778#discussion_r1148305440 PR Review Comment: https://git.openjdk.org/jdk/pull/12778#discussion_r1148305543 PR Review Comment: https://git.openjdk.org/jdk/pull/12778#discussion_r1148305885 PR Review Comment: https://git.openjdk.org/jdk/pull/12778#discussion_r1148306138 PR Review Comment: https://git.openjdk.org/jdk/pull/12778#discussion_r1148306256 PR Review Comment: https://git.openjdk.org/jdk/pull/12778#discussion_r1148306282 PR Review Comment: https://git.openjdk.org/jdk/pull/12778#discussion_r1148306390