feat: add pause support for riscv #1253
Conversation
|
Thank you! I wonder if these changes could be done upstream to cpuid and atomic_queue instead have to maintain 2 patches just for 2 simple changes? |
| @@ -31,6 +31,12 @@ static inline void spin_loop_pause() noexcept { | |||
| defined(__ARM_ARCH_8A__) || \ | |||
| defined(__aarch64__)) | |||
| asm volatile ("yield" ::: "memory"); | |||
| #elif defined(__riscv) | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Let's update to the latest cpuid. I'll make a note of it.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Sure, I'll propose to them hopefully in the next few days
This PR supersedes #1023