RFR: 8362193: Re-work MacOS/AArch64 SpinPause to handle SB [v3]
Aleksey Shipilev
shade at openjdk.org
Tue Jul 22 17:14:58 UTC 2025
On Mon, 21 Jul 2025 20:31:27 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
> ...
>
> Evgeny Astigeevich has updated the pull request incrementally with one additional commit since the last revision:
>
> Put cases in enum order
All right then, looks good to me.
-------------
Marked as reviewed by shade (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/26387#pullrequestreview-3044035998
More information about the hotspot-dev
mailing list