RFR: 8307190: Refactor ref_at methods in Constant Pool [v2]

[Ioi Lam]

On Wed, 17 May 2023 17:40:01 GMT, Matias Saavedra Silva <matsaave at openjdk.org> wrote:

>> In anticipation of [JDK-8301996](https://bugs.openjdk.org/browse/JDK-8301996), some of the accessors in constantpool.cpp need to be updated. The CPCache rework introduces multiple new meanings to the index argument passed to these functions, so they need to be restructured in a way that facilitates different paths depending on the input. For this enhancement, the bytecode is propagated by the callers to determine how to handle the index. Thanks to this and JDK-8307306, `bool uncached` is no longer needed in these functions.
>> 
>> Tests have been altered to suit the changes to JVMCI. Verified with tier1-5 tests.
>
> Matias Saavedra Silva has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision:
> 
>  - Fixed javadoc and test cleanup
>  - Merge branch 'master' into refactor_ref_at_8307190
>  - Fixed comments and copyright
>  - Changed compilerToVM methods
>  - Coleen comments
>  - 8307190: Refactor ref_at methods in Constant Pool

Please double check the Java method parameters again, and rename (cpi, index, which) -> (rawIndex) as appropriate.

src/hotspot/share/jvmci/jvmciRuntime.cpp line 1809:

> 1807: 
> 1808:   // Get the field's name, signature, and type.
> 1809:   Symbol* name  = cpool->name_ref_at(index, Bytecodes::_getfield /*We know it's a field*/);

You should not hard code the bytecode. The bytecode should be passed in by callers such as this place:


ciField* ciBytecodeStream::get_field(bool& will_link) {
  ciField* f = CURRENT_ENV->get_field_by_index(_holder, get_field_index());
  will_link = f->will_link(_method, _bc);
  return f;
}


(Same comment for the other Bytecodes::_getfield in this function.

src/jdk.internal.vm.ci/share/classes/jdk/vm/ci/hotspot/HotSpotConstantPool.java line 396:

> 394:      * @return {@code JVM_CONSTANT_NameAndType} reference constant pool entry
> 395:      */
> 396:     private int getNameAndTypeRefIndexAt(int index, int opcode) {

index -> rawIndex?

src/jdk.internal.vm.ci/share/classes/jdk/vm/ci/hotspot/HotSpotConstantPool.java line 408:

> 406:      * @return name as {@link String}
> 407:      */
> 408:     private String getNameOf(int which, int opcode) {

is which a rawIndex?

src/jdk.internal.vm.ci/share/classes/jdk/vm/ci/hotspot/HotSpotConstantPool.java line 417:

> 415:      *
> 416:      * @param index constant pool index
> 417:      * @param opcode the opcode of the instruction for which the lookup is being performed

There's no opcode parameter to this method.

src/jdk.internal.vm.ci/share/classes/jdk/vm/ci/hotspot/HotSpotConstantPool.java line 457:

> 455:      * @return klass reference index
> 456:      */
> 457:     private int getKlassRefIndexAt(int index, int opcode) {

index should be changed to rawIndex. JavaDoc needs update.

test/hotspot/jtreg/compiler/jvmci/common/patches/jdk.internal.vm.ci/jdk/vm/ci/hotspot/CompilerToVMHelper.java line 110:

> 108: 
> 109:     public static int lookupNameAndTypeRefIndexInPool(ConstantPool constantPool, int cpi, int opcode) {
> 110:         return CTVM.lookupNameAndTypeRefIndexInPool((HotSpotConstantPool) constantPool, cpi, opcode);

cpi -> rawIndex.

test/hotspot/jtreg/compiler/jvmci/compilerToVM/LookupSignatureInPoolTest.java line 116:

> 114:             break;
> 115:           default:
> 116:             throw new Error("Unexpected consant pool entry");

This code is repeated 3 times. It should be consolidated in a method. Maybe :


int ConstantPoolTestsHelper.getDummyOpcode(ConstantTypes cpType)

-------------

Changes requested by iklam (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/13872#pullrequestreview-1431498263
PR Review Comment: https://git.openjdk.org/jdk/pull/13872#discussion_r1197014188
PR Review Comment: https://git.openjdk.org/jdk/pull/13872#discussion_r1197000292
PR Review Comment: https://git.openjdk.org/jdk/pull/13872#discussion_r1197000122
PR Review Comment: https://git.openjdk.org/jdk/pull/13872#discussion_r1196999832
PR Review Comment: https://git.openjdk.org/jdk/pull/13872#discussion_r1196999159
PR Review Comment: https://git.openjdk.org/jdk/pull/13872#discussion_r1197001365
PR Review Comment: https://git.openjdk.org/jdk/pull/13872#discussion_r1197025659