RFR: 8356159: RISC-V: Add Zabha [v5]

Fri May 23 03:41:05 UTC 2025

On Thu, 22 May 2025 11:59:47 GMT, Robbin Ehn <rehn at openjdk.org> wrote:

>> Hi, please consider.
>> 
>> This adds the byte and halfword atomic memory operations (Zabha) - https://github.com/riscv/riscv-zabha.
>> All amo-instructions, except load-reserve and store-conditional, can also be performed on natural aligned half-words and bytes. (i.e. the extension do not add lr.h/b or sc.h/b) This includes amocas if zacas extension is present.
>> 
>> The majority of this patch is to support amocas.h/b. We are now starting to really feel the pain of all these extensions, as CAS:ing 16/8-bits can now be done in three different ways:
>> - lr.w/sc.w 'narrow' CAS (no extension)
>> - amocas.w 'narrow' CAS (Zacas)
>> - amocas.h/b (Zacas + Zabha)
>> 
>> There is no hwprobe support yet.
>> 
>> Ran t1-3 with Zacas+Zabha and t1 without Zabha in qemu.
>> 
>> Thanks, Robbin
>
> 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 nine additional commits since the last revision:
> 
>  - Review comments
>  - Merge branch 'master' into 8356159
>  - Review fix
>  - Bug and review fixes
>  - Merge branch 'master' into 8356159
>  - Revert back to default relaxed
>  - Merge branch 'master' into 8356159
>  - Fixed ws
>  - Initial draft

Thanks for the update. Seems some more code cleanup could be done after another look.

src/hotspot/cpu/riscv/riscv.ad line 5233:

> 5231: // these have higher priority than the rules selected by a predicate
> 5232: instruct compareAndSwapB_narrow(iRegINoSp res, indirect mem, iRegI_R12 oldval, iRegI_R13 newval,
> 5233:                          iRegINoSp tmp1, iRegINoSp tmp2, iRegINoSp tmp3, rFlagsReg cr)

Indentation.

src/hotspot/cpu/riscv/riscv.ad line 5257:

> 5255: %}
> 5256: 
> 5257: instruct compareAndSwapB(iRegINoSp res, indirect mem, iRegI_R12 oldval, iRegI_R13 newval, rFlagsReg cr)

Like `compareAndSwapI` and `compareAndSwapL`, I don't think we need to make `oldval` or `newval` fixed registers for `compareAndSwapB`. We can remove this constraint and there's no need to list `cr`. I mean this:

instruct compareAndSwapB(iRegINoSp res, indirect mem, iRegI oldval, iRegI newval)

Similar for `compareAndSwapBAcq`, `compareAndSwapS`, `compareAndSwapSAcq` and the weak variants.

src/hotspot/cpu/riscv/riscv.ad line 5280:

> 5278: 
> 5279: instruct compareAndSwapS_narrow(iRegINoSp res, indirect mem, iRegI_R12 oldval, iRegI_R13 newval,
> 5280:                          iRegINoSp tmp1, iRegINoSp tmp2, iRegINoSp tmp3, rFlagsReg cr)

Indentation.

src/hotspot/cpu/riscv/riscv.ad line 5412:

> 5410: // alternative CompareAndSwapX when we are eliding barriers
> 5411: instruct compareAndSwapBAcq_narrow(iRegINoSp res, indirect mem, iRegI_R12 oldval, iRegI_R13 newval,
> 5412:                             iRegINoSp tmp1, iRegINoSp tmp2, iRegINoSp tmp3, rFlagsReg cr)

Indentation.

src/hotspot/cpu/riscv/riscv.ad line 5459:

> 5457: 
> 5458: instruct compareAndSwapSAcq_narrow(iRegINoSp res, indirect mem, iRegI_R12 oldval, iRegI_R13 newval,
> 5459:                          iRegINoSp tmp1, iRegINoSp tmp2, iRegINoSp tmp3, rFlagsReg cr)

Indentation.

src/hotspot/cpu/riscv/riscv.ad line 5600:

> 5598: // sc_d(w) with rl bit set.
> 5599: instruct compareAndExchangeB_narrow(iRegINoSp res, indirect mem, iRegI_R12 oldval, iRegI_R13 newval,
> 5600:                              iRegINoSp tmp1, iRegINoSp tmp2, iRegINoSp tmp3, rFlagsReg cr)

Indentation.

src/hotspot/cpu/riscv/riscv.ad line 5623:

> 5621: %}
> 5622: 
> 5623: instruct compareAndExchangeB(iRegINoSp res, indirect mem, iRegI_R12 oldval, iRegI_R13 newval, rFlagsReg cr)

Similar here. No need to make `oldval` or `newval` fixed registers for `compareAndExchangeB`, `compareAndExchangeS`, `compareAndExchangeBAcq` or `compareAndExchangeSAcq`. And we should also remove `cr` which is not used.

src/hotspot/cpu/riscv/riscv.ad line 5644:

> 5642: 
> 5643: instruct compareAndExchangeS_narrow(iRegINoSp res, indirect mem, iRegI_R12 oldval, iRegI_R13 newval,
> 5644:                              iRegINoSp tmp1, iRegINoSp tmp2, iRegINoSp tmp3, rFlagsReg cr)

Indentation.

src/hotspot/cpu/riscv/riscv.ad line 5764:

> 5762: 
> 5763: instruct compareAndExchangeBAcq_narrow(iRegINoSp res, indirect mem, iRegI_R12 oldval, iRegI_R13 newval,
> 5764:                                 iRegINoSp tmp1, iRegINoSp tmp2, iRegINoSp tmp3, rFlagsReg cr)

Indentation.

src/hotspot/cpu/riscv/riscv.ad line 5808:

> 5806: 
> 5807: instruct compareAndExchangeSAcq_narrow(iRegINoSp res, indirect mem, iRegI_R12 oldval, iRegI_R13 newval,
> 5808:                                 iRegINoSp tmp1, iRegINoSp tmp2, iRegINoSp tmp3, rFlagsReg cr)

Indentation.

src/hotspot/cpu/riscv/riscv.ad line 5932:

> 5930: 
> 5931: instruct weakCompareAndSwapB_narrow(iRegINoSp res, indirect mem, iRegI_R12 oldval, iRegI_R13 newval,
> 5932:                              iRegINoSp tmp1, iRegINoSp tmp2, iRegINoSp tmp3, rFlagsReg cr)

Indentation.

src/hotspot/cpu/riscv/riscv.ad line 5978:

> 5976: 
> 5977: instruct weakCompareAndSwapS_narrow(iRegINoSp res, indirect mem, iRegI_R12 oldval, iRegI_R13 newval,
> 5978:                              iRegINoSp tmp1, iRegINoSp tmp2, iRegINoSp tmp3, rFlagsReg cr)

Indentation.

src/hotspot/cpu/riscv/riscv.ad line 6104:

> 6102: 
> 6103: instruct weakCompareAndSwapBAcq_narrow(iRegINoSp res, indirect mem, iRegI_R12 oldval, iRegI_R13 newval,
> 6104:                                 iRegINoSp tmp1, iRegINoSp tmp2, iRegINoSp tmp3, rFlagsReg cr)

Indentation.

src/hotspot/cpu/riscv/riscv.ad line 6150:

> 6148: 
> 6149: instruct weakCompareAndSwapSAcq_narrow(iRegINoSp res, indirect mem, iRegI_R12 oldval, iRegI_R13 newval,
> 6150:                                 iRegINoSp tmp1, iRegINoSp tmp2, iRegINoSp tmp3, rFlagsReg cr)

Indentation.

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

PR Review: https://git.openjdk.org/jdk/pull/25252#pullrequestreview-2863023510
PR Review Comment: https://git.openjdk.org/jdk/pull/25252#discussion_r2103704579
PR Review Comment: https://git.openjdk.org/jdk/pull/25252#discussion_r2103718941
PR Review Comment: https://git.openjdk.org/jdk/pull/25252#discussion_r2103704778
PR Review Comment: https://git.openjdk.org/jdk/pull/25252#discussion_r2103704959
PR Review Comment: https://git.openjdk.org/jdk/pull/25252#discussion_r2103705138
PR Review Comment: https://git.openjdk.org/jdk/pull/25252#discussion_r2103705235
PR Review Comment: https://git.openjdk.org/jdk/pull/25252#discussion_r2103721902
PR Review Comment: https://git.openjdk.org/jdk/pull/25252#discussion_r2103705329
PR Review Comment: https://git.openjdk.org/jdk/pull/25252#discussion_r2103705446
PR Review Comment: https://git.openjdk.org/jdk/pull/25252#discussion_r2103705515
PR Review Comment: https://git.openjdk.org/jdk/pull/25252#discussion_r2103705605
PR Review Comment: https://git.openjdk.org/jdk/pull/25252#discussion_r2103705679
PR Review Comment: https://git.openjdk.org/jdk/pull/25252#discussion_r2103705760
PR Review Comment: https://git.openjdk.org/jdk/pull/25252#discussion_r2103705822