Skip to content

Add feature gate for atomic load/store, gate spsc behind loadstore (Alternative fix for #271) #273

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 1 commit into from

Conversation

Kriskras99
Copy link

The spsc module should only be build when atomic_polyfill is available. It's a big dependency (pulls in 12 extra dependencies) so for projects that don't need the atomics it's better to hide it behind a (default) feature gate.

I added two new features:

  • loadstore that pulls in atomic_polyfill for architectures that need it and gates spsc
  • atomics that enables loadstone and cas (replaces cas as the default feature)

Doing it this way should allow for more granular configuration of architectures, i.e. only pull-in atomic_polyfill if an architecture uses loadstore and doesn't have native support. This pull request does not contain that granularity as I'm not familiar with other architectures.

@TDHolmes
Copy link
Contributor

TDHolmes commented Mar 8, 2022

But this will now unnecessarily pull in atomic-polyfill even for targets with native atomic support

@Kriskras99
Copy link
Author

It only gets pulled in for targets which have 'atomic_polufill' as an (optional) dependency. For targets that support load store natively you can use cfgs in the Cargo.toml to only use 'atomic_polyfill' for 'cas'

@Kriskras99
Copy link
Author

It only gets pulled in for targets which have 'atomic_polufill' as an (optional) dependency. For targets that support load store natively you can use cfgs in the Cargo.toml to only use 'atomic_polyfill' for 'cas'

This is slightly wrong. It's true that specifying atomic_polyfill as a feature only pulls it in as an optional dependency. However it's not possible to only pull it in for cas for some architectures. For that we'd need something like this.

bors bot added a commit that referenced this pull request May 12, 2022
293: unconditionally depend on atomic-polyfill for riscv32i target r=japaric a=japaric

due to a limitation in the llvm backend [1] the `riscv32i-unknown-none-elf` target lacks the `core::sync::atomic` API even though the actual hardware is capable of atomic loads and stores (`fence` instruction). thus, at this point in time, this target needs `atomic-polyfill` for a working SPSC implementation

[1]: knurling-rs/defmt#597 (comment)

fixes #271 
closes #272 
closes #273 

Co-authored-by: Jorge Aparicio <[email protected]>
bors bot added a commit that referenced this pull request May 12, 2022
293: unconditionally depend on atomic-polyfill for riscv32i target r=japaric a=japaric

due to a limitation in the llvm backend [1] the `riscv32i-unknown-none-elf` target lacks the `core::sync::atomic` API even though the actual hardware is capable of atomic loads and stores (`fence` instruction). thus, at this point in time, this target needs `atomic-polyfill` for a working SPSC implementation

[1]: knurling-rs/defmt#597 (comment)

fixes #271 
closes #272 
closes #273 

Co-authored-by: Jorge Aparicio <[email protected]>
@bors bors bot closed this in #293 May 12, 2022
@bors bors bot closed this in 02773a5 May 12, 2022
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