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.

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

Conversation

XieJiSS
Copy link

@XieJiSS XieJiSS commented Feb 28, 2024

This PR supersedes #1023

@redtide
Copy link
Member

redtide commented Feb 28, 2024

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)
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

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants