Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 14 commits into from
Closed
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6816,6 +6816,7 @@ void MacroAssembler::spin_wait() {
yield();
break;
case SpinWait::SB:
assert(VM_Version::supports_sb(), "current CPU does not support SB instruction");
sb();
break;
default:
Expand Down
51 changes: 51 additions & 0 deletions src/hotspot/cpu/aarch64/spin_wait_aarch64.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
/*
* Copyright Amazon.com Inc. or its affiliates. All Rights Reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/

#include "spin_wait_aarch64.hpp"

#include <string.h>

bool SpinWait::supports(const char *name) {
return name != nullptr &&
(strcmp(name, "nop") == 0 ||
strcmp(name, "isb") == 0 ||
strcmp(name, "yield") == 0 ||
strcmp(name, "sb") == 0 ||
strcmp(name, "none") == 0);
}

SpinWait::Inst SpinWait::from_name(const char* name) {
assert(supports(name), "spin wait instruction name must be one of: " SPIN_WAIT_INST_OPTIONS);

if (strcmp(name, "nop") == 0) {
return SpinWait::NOP;
} else if (strcmp(name, "isb") == 0) {
return SpinWait::ISB;
} else if (strcmp(name, "yield") == 0) {
return SpinWait::YIELD;
} else if (strcmp(name, "sb") == 0) {
return SpinWait::SB;
}

return SpinWait::NONE;
}
24 changes: 17 additions & 7 deletions src/hotspot/cpu/aarch64/spin_wait_aarch64.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -19,31 +19,41 @@
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*
*/

#ifndef CPU_AARCH64_SPIN_WAIT_AARCH64_HPP
#define CPU_AARCH64_SPIN_WAIT_AARCH64_HPP

#define SPIN_WAIT_INST_OPTIONS "nop, isb, yield, sb, none"

class SpinWait {
public:
// Non-zero values are chosen to have only one bit set.
// This simplifies testing values in assembly code.
// This limits us to 64 possible implementation.
// Value 1 is used for the default implementation.
enum Inst {
NONE = -1,
NOP,
ISB,
YIELD,
SB
NONE = 0,
YIELD = (1 << 0),
ISB = (1 << 1),
SB = (1 << 2),
NOP = (1 << 3)
};

private:
Inst _inst;
int _count;

Inst from_name(const char *name);

public:
SpinWait(Inst inst = NONE, int count = 0) : _inst(inst), _count(count) {}
SpinWait(Inst inst = NONE, int count = 0) : _inst(inst), _count(inst == NONE ? 0 : count) {}
SpinWait(const char *name, int count) : SpinWait(from_name(name), count) {}

Inst inst() const { return _inst; }
int inst_count() const { return _count; }

static bool supports(const char *name);
};

#endif // CPU_AARCH64_SPIN_WAIT_AARCH64_HPP
26 changes: 10 additions & 16 deletions src/hotspot/cpu/aarch64/vm_version_aarch64.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -51,26 +51,20 @@ uintptr_t VM_Version::_pac_mask;
SpinWait VM_Version::_spin_wait;

static SpinWait get_spin_wait_desc() {
if (strcmp(OnSpinWaitInst, "nop") == 0) {
return SpinWait(SpinWait::NOP, OnSpinWaitInstCount);
} else if (strcmp(OnSpinWaitInst, "isb") == 0) {
return SpinWait(SpinWait::ISB, OnSpinWaitInstCount);
} else if (strcmp(OnSpinWaitInst, "yield") == 0) {
return SpinWait(SpinWait::YIELD, OnSpinWaitInstCount);
} else if (strcmp(OnSpinWaitInst, "sb") == 0) {
if (!VM_Version::supports_sb()) {
vm_exit_during_initialization("OnSpinWaitInst is SB but current CPU does not support SB instruction");
}
return SpinWait(SpinWait::SB, OnSpinWaitInstCount);
} else if (strcmp(OnSpinWaitInst, "none") != 0) {
vm_exit_during_initialization("The options for OnSpinWaitInst are nop, isb, yield, sb, and none", OnSpinWaitInst);
if (!SpinWait::supports(OnSpinWaitInst)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is not about the actual spin-wait hints support, correct? This only checks the option string is in expected domain? A common way to deal with this is to use constraint functions, see for example:

  product(ccstr, AOTCache, nullptr,                                         \
          "Cache for improving start up and warm up")                       \
          constraint(AOTCacheConstraintFunc, AtParse)                       \
                                                                            \

vm_exit_during_initialization("OnSpinWaitInst is not one of "
SPIN_WAIT_INST_OPTIONS,
OnSpinWaitInst);
}

if (!FLAG_IS_DEFAULT(OnSpinWaitInstCount) && OnSpinWaitInstCount > 0) {
vm_exit_during_initialization("OnSpinWaitInstCount cannot be used for OnSpinWaitInst 'none'");
assert(OnSpinWaitInstCount != 0, "allowed range for OnSpinWaitInstCount must not include 0");

SpinWait spin_wait(OnSpinWaitInst, OnSpinWaitInstCount);
if (spin_wait.inst() == SpinWait::SB && !VM_Version::supports_sb()) {
vm_exit_during_initialization("OnSpinWaitInst is SB but current CPU does not support SB instruction");
}

return SpinWait{};
return spin_wait;
}

void VM_Version::initialize() {
Expand Down
73 changes: 46 additions & 27 deletions src/hotspot/os_cpu/bsd_aarch64/os_bsd_aarch64.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@
#include "runtime/sharedRuntime.hpp"
#include "runtime/stubRoutines.hpp"
#include "runtime/timer.hpp"
#include "runtime/vm_version.hpp"
#include "signals_posix.hpp"
#include "utilities/align.hpp"
#include "utilities/events.hpp"
Expand Down Expand Up @@ -524,39 +525,57 @@ static inline void atomic_copy64(const volatile void *src, volatile void *dst) {
}

extern "C" {
// needs local assembler label '1:' to avoid trouble when using linktime optimization
int SpinPause() {
// We don't use StubRoutines::aarch64::spin_wait stub in order to
// avoid a costly call to os::current_thread_enable_wx() on MacOS.
// We should return 1 if SpinPause is implemented, and since there
// will be a sequence of 11 instructions for NONE and YIELD and 12
// instructions for NOP and ISB, SpinPause will always return 1.
uint64_t br_dst;
const int instructions_per_case = 2;
int64_t off = VM_Version::spin_wait_desc().inst() * instructions_per_case * Assembler::instruction_size;

assert(VM_Version::spin_wait_desc().inst() >= SpinWait::NONE &&
VM_Version::spin_wait_desc().inst() <= SpinWait::YIELD, "must be");
assert(-1 == SpinWait::NONE, "must be");
assert( 0 == SpinWait::NOP, "must be");
assert( 1 == SpinWait::ISB, "must be");
assert( 2 == SpinWait::YIELD, "must be");
// will be always a sequence of instructions, SpinPause will always return 1.

assert(SpinWait::NONE == 0, "SpinWait::Inst value 0 reserved to indicate no implementation");
assert(SpinWait::YIELD == 1, "SpinWait::Inst value 1 reserved for 'yield' instruction");
assert(SpinWait::ISB == 2, "SpinWait::Inst value 2 reserved for 'isb' instruction");
assert(SpinWait::SB == 4, "SpinWait::Inst value 4 reserved for 'sb' instruction");
assert(SpinWait::NOP == 8, "SpinWait::Inst value 8 reserved for 'nop' instruction");
assert(inst_id == 0 || is_power_of_2((uint64_t)inst_id), "Values of SpinWait::Inst must be 0 or power of 2");
assert(inst_id != SpinWait::SB || VM_Version::supports_sb(), "current CPU does not support SB instruction");

if (inst_id < SpinWait::NONE || inst_id > SpinWait::NOP) {
warining("Unsupported type of SpinWait::Inst: %d", inst_id);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning

Also, I think we want to minimize the amount of code we actually execute in SpinPause, since it likely sits in the hot loop. So checking this here is probably counter-productive.

ShouldNotReachHere();
}

// The assembly code below is equivalent to the following:
//
// if (inst_id == 1) {
// exec_yield_inst();
// } else if (inst_id == 2) {
// exec_isb_inst();
// } else if (inst_id == 4) {
// exec_sb_inst();
// } else if (inst_id == 8) {
// exec_nop_inst();
// }
asm volatile(
" adr %[d], 20 \n" // 20 == PC here + 5 instructions => address
// to entry for case SpinWait::NOP
" add %[d], %[d], %[o] \n"
" br %[d] \n"
" b 1f \n" // case SpinWait::NONE (-1)
" nop \n" // padding
" nop \n" // case SpinWait::NOP ( 0)
" b 1f \n"
" isb \n" // case SpinWait::ISB ( 1)
" b 1f \n"
" yield \n" // case SpinWait::YIELD ( 2)
"1: \n"
: [d]"=&r"(br_dst)
: [o]"r"(off)
" tbz %[id], 0, 0f \n" // The default instruction for SpinWait is YIELD.
// We check it first before going to switch.
" yield \n"
" b 4f \n"
"0: \n"
" tbnz %[id], 1, 1f \n"
" tbnz %[id], 2, 2f \n"
" tbnz %[id], 3, 3f \n"
" b 4f \n"
"1: \n"
" isb \n"
" b 4f \n"
"2: \n"
" .inst 0xd50330ff \n" // SB instruction, explicitly encoded not to rely on a compiler
" b 4f \n"
"3: \n"
" nop \n"
"4: \n"
:
: [id]"r"((uint64_t)inst_id)
: "memory");
return 1;
}
Expand Down
33 changes: 33 additions & 0 deletions test/hotspot/gtest/aarch64/test_spin_pause.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
/*
* Copyright Amazon.com Inc. or its affiliates. All Rights Reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/

#if defined(AARCH64) && !defined(ZERO)

#include "utilities/spinYield.hpp"
#include "unittest.hpp"

TEST_VM(SpinPause, sanity) {
ASSERT_EQ(SpinPause(), 1);
}

#endif // AARCH64
46 changes: 46 additions & 0 deletions test/hotspot/jtreg/gtest/TestSpinPauseAArch64.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
/*
* Copyright Amazon.com Inc. or its affiliates. All Rights Reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/

/*
* @test TestSpinPauseAArch64
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A common way to tag the tests:

@test id=default

I assume all these are supported in ARMv8.0 (default? baseline?) profile. That's why I put default. Put something else if that is incorrect.

Same for the second @test block.

* @bug 8362193
* @summary Run SpinPause gtest using different instructions for SpinPause
* @library /test/lib
* @requires vm.flagless
* @requires os.arch=="aarch64"
* @run main/native GTestWrapper --gtest_filter=SpinPause*
* @run main/native GTestWrapper --gtest_filter=SpinPause* -XX:+UnlockDiagnosticVMOptions -XX:OnSpinWaitInst=none
* @run main/native GTestWrapper --gtest_filter=SpinPause* -XX:+UnlockDiagnosticVMOptions -XX:OnSpinWaitInst=nop
* @run main/native GTestWrapper --gtest_filter=SpinPause* -XX:+UnlockDiagnosticVMOptions -XX:OnSpinWaitInst=isb
* @run main/native GTestWrapper --gtest_filter=SpinPause* -XX:+UnlockDiagnosticVMOptions -XX:OnSpinWaitInst=yield
*/

/*
* @test TestSpinPauseSBAArch64
* @bug 8362193
* @summary Run SpinPause gtest using SB instruction for SpinPause
* @library /test/lib
* @requires vm.flagless
* @requires (os.arch=="aarch64" & vm.cpu.features ~= ".*sb.*")
* @run main/native GTestWrapper --gtest_filter=SpinPause* -XX:+UnlockDiagnosticVMOptions -XX:OnSpinWaitInst=sb
*/