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

[Robbin Ehn]

On Tue, 2 Sep 2025 12:48:11 GMT, Hamlin Li <mli at openjdk.org> wrote:

>> From JBS entry, the point is to do it in a sane order:
>> 
>> The release in make_jal_opt so to make sure the store to instruction stream happens before I-cache flush.
>> 
>> 1: store destination to stub
>> 2: release
>> 3: store destination to instruction stream
>> 4: release
>> 5: i-cache flush
>
> I don't see a detailed discussion about why there needs to be 2 `release`.
> Seems the `2: release` is redundant? does a single release (step 4) after step 3 work as well?

Regarding 4:
Now, from this code perspective i-cache invalidate is a bit opaque.
We do know that we don't want the store to happen after the flush.
The risc-v implementation do emit a full fence before flush, as stores may be reordered over fence.i.
But the AbstractICache::invalidate_range is not documented to guarantee to have this effect.

Regarding 2:
If someone executes the new instruction when changed to jalr(3), we did want them to call the new location we stored to the stub(1). By saying 1 happens before 3, we convey our intent.
Aarch64 also have this.

So non of the releases (2,4) is truly need AFIACT, as this code must support both calling old dest and new dest.
E.g. if you are context switch after loading old dest, context switch back and executes jalr, you will be calling old dest, which is fine as that method is marked not-entrant. Causing you to resolve this call then you will see the new dest.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/26944#discussion_r2318105086