RFR: 8362193: Re-work MacOS/AArch64 SpinPause to handle SB

[Andrew Haley]

On Fri, 18 Jul 2025 13:11:26 GMT, Evgeny Astigeevich <eastigeevich at openjdk.org> wrote:

> ### Background
> 
> With JDK-8359435 "AArch64: add support for SB instruction to MacroAssembler::spin_wait" we have an option to use the speculation barrier (SB) instruction for `j.l.Thread::onSpinWait` and `SpinPause`. On Linux AArch64 `SpinPause` uses a stub which is generated  with `MacroAssembler::spin_wait`. `j.l.Thread::onSpinWait` uses it as well. As a result tests for `j.l.Thread::onSpinWait`, e.g.`compiler/onSpinWait/TestOnSpinWaitAArch64.java`, cover both `j.l.Thread::onSpinWait` and `SpinPause`. Also we don't need to update `SpinPause` when we add support for a new instruction to `MacroAssembler::spin_wait`.
> 
> On Mac AArch64 `SpinPause` does not use the generated stub to  avoid a costly call to `os::current_thread_enable_wx()`. It uses inline assembly instead. As a result we have `SpinWait` implementation details leaking into `SpinPause`. Besides asserts there are no tests covering Mac implementation of `SpinPause`. Testing on Apple M3 Pro showed that `compiler/onSpinWait/TestOnSpinWaitAArch64.java` could not reliably trigger those asserts. `ArchiveWorkers::run_task_multi` did not invoke `spin.wait()`. The inline assembly in Mac AArch64 `SpinPause` has another issue. It uses a jump table. An offset in the table is calculated based on the value of `enum SpinWait::Inst`. When a new value `SB` was added the offset became out of bounds. The jump goes out of the assembly code.
> 
> ### Summary of changes
> 
> * Rewrote Mac AArch64 `SpinPause` not to use a jump table.
> * Added `SpinWait::supports()` and `SpinWait::from_name()` methods to validate and convert instruction names into corresponding enum values.
> * Updated `SpinWait::Inst` enum to use bit flags for simplified assembly testing and added a constructor to initialize `SpinWait` using instruction names.
> * `src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp`: Added an assertion to ensure the `sb` instruction is supported by the CPU before generation.
> * Added `OnSpinWaitInstNameConstraintFunc` for the `OnSpinWaitInst` option to check option's value at parsing.
> * Added a gtest to verify the functionality of `SpinPause`.
> * Added a jtreg test to run the gtest for`SpinPause` with various instructions, including the `sb` instruction if supported by the CPU.
> 
> ### Testing results: fastdebug, release
> 
> - Linux, Graviton 2
>    - tier1: Passed
>    - `test/hotspot/jtreg/gtest/TestSpinPauseAArch64.java`: Passed
>    - `test/hotspot/jtreg/compiler/onSpinWait`: Passed
> - MacOS, M3 Pro
>    - tier1, OnSpinWaitInst=default: Passed
>    ...

src/hotspot/os_cpu/bsd_aarch64/os_bsd_aarch64.cpp line 577:

> 575:         "4:                      \n"
> 576:         :
> 577:         : [id]"r"(inst_id)

There's no good reason to handle all this logic in asm. Please use a switch statement instead, and we can also get rid of most of the assertions by adding a ShouldNotReachHere() in the default clause.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/26387#discussion_r2218554094