Skip to content
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.

Sign up for GitHub

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

feat: add pause support for riscv #1253

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

feat: add pause support for riscv #1253

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3660086
feat: add riscv detection macros in config.hpp
XieJiSS Feb 28, 2024
2780c18
feat: add pause support for riscv
XieJiSS Feb 28, 2024
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
6 changes: 6 additions & 0 deletions external/atomic_queue/include/atomic_queue/defs.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,12 @@ static inline void spin_loop_pause() noexcept {
defined(__ARM_ARCH_8A__) || \
defined(__aarch64__))
asm volatile ("yield" ::: "memory");
#elif defined(__riscv)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is still within the #elif defined(__arm__) || defined(__aarch64__) above, so it will never actually apply. Also, the upstream atomic_queue already added RISC-V support with something that looks potentially equivalent to what you've added here.

Copy link
Author

Choose a reason for hiding this comment

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

Yes the upstream version is indeed the same thing as this one, despite that the .insn i pseudo-instruction is not supported until llvm >= 14 AFAIK

So basically if we use upstream's implementation then we'll be dropping support of some relatively older (but actually not that old) compilers. Which I think is okay because most RISC-V users know how to upgrade their compiler, but I'd like to know @redtide's opinion

Copy link
Member

Choose a reason for hiding this comment

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

I think it's up to @paulfd to decide on this, IMO I would not care much about "old" compilers and use upstream to not to have to maintain patched stuff, so it would be nice to me if the upstream atomic_queue could be used as is and patch cpuid instead.

Copy link
Member

Choose a reason for hiding this comment

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

Let's update to the latest cpuid. I'll make a note of it.

Copy link
Member

@redtide redtide Mar 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Let's update to the latest cpuid. I'll make a note of it.

There is #1249 on the way but I wasn't precise: with "patch cpuid instead" I meant "patch cpuid upstream (platform repo) instead", so @XieJiSS, could you please propose your cpuid changes to them?

Copy link
Author

Choose a reason for hiding this comment

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

Sure, I'll propose to them hopefully in the next few days

Copy link
Member

Choose a reason for hiding this comment

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

Thank you very much!

#if defined(__riscv_zihintpause)
asm volatile ("pause" ::: "memory");
#else
/* Encoding of the pause instruction, will be treated as nop if not supported by hardware */
asm volatile (".4byte 0x100000F");
#else
asm volatile ("nop" ::: "memory");
#endif
Expand Down
8 changes: 8 additions & 0 deletions src/external/cpuid/platform/src/platform/config.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,10 @@
#define PLATFORM_MIPS 1
#define PLATFORM_CLANG_MIPS 1
#define PLATFORM_GCC_COMPATIBLE_MIPS 1
#elif defined(__riscv)
#define PLATFORM_RISCV 1
#define PLATFORM_CLANG_RISCV 1
#define PLATFORM_GCC_COMPATIBLE_RISCV 1
#elif defined(__asmjs__)
#define PLATFORM_ASMJS 1
#define PLATFORM_CLANG_ASMJS 1
Expand All @@ -75,6 +79,10 @@
#define PLATFORM_MIPS 1
#define PLATFORM_GCC_MIPS 1
#define PLATFORM_GCC_COMPATIBLE_MIPS 1
#elif defined(__riscv)
#define PLATFORM_RISCV 1
#define PLATFORM_GCC_RISCV 1
#define PLATFORM_GCC_COMPATIBLE_RISCV 1
#endif
#elif defined(_MSC_VER)
#define PLATFORM_MSVC 1
Expand Down
Loading