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

Add replace and set methods for CPU local variables #1015

Closed

Conversation

tsoutsman
Copy link
Member

Only works for CPU-locals under 8 bytes, which happens to be all we need. This implementation doesn't need to disable preemption.

Depends on #1012.

Almost ready, just needs some cleanup (renaming stuff, docs, safety comments, etc.).

Future PRs:

  • Dynamic .cls section
  • Remove per_cpu and cpu_local

Signed-off-by: Klimenty Tsoutsman <[email protected]>
Signed-off-by: Klimenty Tsoutsman <[email protected]>
Signed-off-by: Klimenty Tsoutsman <[email protected]>
Signed-off-by: Klimenty Tsoutsman <[email protected]>
Signed-off-by: Klimenty Tsoutsman <[email protected]>
@tsoutsman tsoutsman marked this pull request as draft July 24, 2023 09:24
@tsoutsman tsoutsman changed the title Add replace and set methods for CPU-locals. Add replace and set methods for CPU local variables Jul 24, 2023
tsoutsman added 12 commits July 24, 2023 19:34
Signed-off-by: Klimenty Tsoutsman <[email protected]>
And remove some cheeky undefined behaviour 🥴.

Signed-off-by: Klimenty Tsoutsman <[email protected]>
Signed-off-by: Klimenty Tsoutsman <[email protected]>
Signed-off-by: Klimenty Tsoutsman <[email protected]>
Signed-off-by: Klimenty Tsoutsman <[email protected]>
Signed-off-by: Klimenty Tsoutsman <[email protected]>
Signed-off-by: Klimenty Tsoutsman <[email protected]>
@kevinaboos
Copy link
Member

@tsoutsman #1012 has been merged in, thanks!

@tsoutsman tsoutsman requested a review from kevinaboos July 27, 2023 23:21
@tsoutsman tsoutsman marked this pull request as ready for review July 27, 2023 23:22
Copy link
Member

@kevinaboos kevinaboos left a comment

Choose a reason for hiding this comment

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

This is an interesting approach, marshalling objects into and out of a single u64. I'm in favor of that but only for cases where it's fully safe, at least initially.

While I did review the code and believe everything is correct and working, and definitely appreciate the ideas herein, I'm not a huge fan of this very raw approach since it goes against the "safety-first" design ethos of Theseus. I'm all for a trait-based design that restricts the kind of types one can place into CLS, e.g., types that are Copy + Into<u64> + From<u64> and maybe other less-restrictive bounds. However, this design puts a huge onus on the users of the cls_macro to get the from_raw()/into_raw() implementation right -- and those are quite tricky. If we introduce more complex types, those implementations will get continually harder to nail down, and they also require a large maintenance burden any time the main type changes.

Another aspect of Theseus's design principles I'd like to highlight is the "intralingual" one, which here can essentially just be interpreted as "be as safe as possible" and "reuse existing types and language features as much as possible". That's why i wanted to explore the idea of using TLS here for CLS in the first place. This raw-based approach is about as extralingual as one can get (in that the compiler cannot really leverage its type checks to ensure that we did the raw conversions correctly), which also contributes to my hesitancy

Perhaps this represents an intermediary step towards a safer version of CLS? If you have such a design path in mind, feel free to share it. Happy to discuss here or on discord as you prefer.

Comment on lines +11 to +13
// TODO: Support multiple integer sizes?
/// Trait for types that can be stored in a single register.
pub trait Raw {
Copy link
Member

@kevinaboos kevinaboos Jul 28, 2023

Choose a reason for hiding this comment

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

I don't think we need too much more type-specific support, but what we can do is add an implementation of Raw for anything that implements Into<u64> + From<u64> -- that should cover pretty much all the relevant desired primitive-ish types.

@tsoutsman tsoutsman closed this Aug 2, 2023
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.

2 participants