RFR: 8365926: RISC-V: Performance regression in renaissance (chi-square) [v2]

[Hamlin Li]

On Mon, 1 Sep 2025 08:56:25 GMT, Robbin Ehn <rehn at openjdk.org> wrote:

>> Hey, please consider!
>> 
>> A bunch of info in JBS entry, please read that also.
>> 
>> I narrowed this issue down to the old jal optimization, making direct calls when in reach.
>> This patch restores them and removes this regression.
>> 
>> In essence we turn "jalr ra,0(t1)" into a "jal ra,<dest>" if reachable, and restore the jalr if a new destination is not reachable.
>> 
>> Please test on your hardware!
>> 
>> 
>> Chi Square (100 runs each, 10 fastest iterations of each run, P550)
>> JDK-23 (last version with trampoline calls)
>> Mean: 3189.5827
>> Standard Deviation: 284.6478
>> 
>> JDK-25
>> Mean: 3424.8905
>> Standard Deviation: 222.2208
>> 
>> Patch:
>> Mean: 3144.8535
>> Standard Deviation: 229.2577
>> 
>> 
>> No issues found in t1, running t2 also. Stress tested on vf2, bpi-f3, p550.
>
> Robbin Ehn 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 four additional commits since the last revision:
> 
>  - Merge branch 'master' into 8365926
>  - Spelling
>  - Merge branch 'master' into 8365926
>  - draft jal<->jalr

src/hotspot/cpu/riscv/nativeInst_riscv.cpp line 60:

> 58:   assert(cb != nullptr && cb->is_nmethod(), "nmethod expected");
> 59:   nmethod *nm = (nmethod *)cb;
> 60:   assert(nm != nullptr, "Sanity");

This line can be removed.

src/hotspot/cpu/riscv/nativeInst_riscv.cpp line 62:

> 60:   assert(nm != nullptr, "Sanity");
> 61:   assert(nm->stub_contains(stub_addr), "Sanity");
> 62:   assert(stub_addr!= nullptr, "Sanity");

Suggestion:

  assert(stub_addr != nullptr, "Sanity");

src/hotspot/cpu/riscv/nativeInst_riscv.cpp line 95:

> 93:   // Skip over auipc + ld
> 94:   address jal_pc = instruction_address() + 2 * NativeInstruction::instruction_size;
> 95:   uint32_t *jal_pos = (uint32_t *)jal_pc;

Is it possible to lose some data in this conversion?
If not, maybe an assert here?

src/hotspot/cpu/riscv/nativeInst_riscv.cpp line 103:

> 101:   } else if (!MacroAssembler::is_jalr_at(jal_pc)) { // The jalr is always identical: jalr ra, 0(t1)
> 102:     uint32_t new_jal = Assembler::encode_jalr(ra, t1, 0);
> 103:     Atomic::store(jal_pos, new_jal);

Suggestion:

    uint32_t new_jalr = Assembler::encode_jalr(ra, t1, 0);
    Atomic::store(jal_pos, new_jalr);

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

PR Review Comment: https://git.openjdk.org/jdk/pull/26944#discussion_r2313464396
PR Review Comment: https://git.openjdk.org/jdk/pull/26944#discussion_r2313464121
PR Review Comment: https://git.openjdk.org/jdk/pull/26944#discussion_r2313463904
PR Review Comment: https://git.openjdk.org/jdk/pull/26944#discussion_r2313463847