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

[Evgeny Astigeevich]

On Mon, 21 Jul 2025 08:49:31 GMT, Andrew Haley <aph 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
> ...
>
> 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.

I agree with you. I proposed to use the switch when JDK-8321371 was being reviewed: https://github.com/openjdk/jdk/pull/16994#issuecomment-1865147655

Frederick (@fbredber) wanted to avoid branches: https://github.com/openjdk/jdk/pull/16994#issuecomment-1865955740

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

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