RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry [v13]

[Fei Yang]

On Fri, 24 Mar 2023 15:13:40 GMT, Matias Saavedra Silva <matsaave at 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