On Fri, 24 Mar 2023 15:13:40 GMT, Matias Saavedra Silva <[email protected]>
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
