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

Jump to bottom

Make compatible with thumbv6m-none-eabi #1384

Merged
merged 1 commit into from
Aug 2, 2024
+47 −0
Merged

Make compatible with thumbv6m-none-eabi #1384

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
21 changes: 21 additions & 0 deletions .github/workflows/ci.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,27 @@ jobs:
components: rustfmt
- run: cargo fmt --all --check

nostd-build:
runs-on: ubuntu-latest
continue-on-error: ${{ matrix.experimental }}
strategy:
matrix:
include:
- rust: stable
experimental: false
target: thumbv6m-none-eabi

name: nostd-build/${{ matrix.target }}/${{ matrix.rust }}
steps:
- uses: actions/checkout@v4
- uses: dtolnay/rust-toolchain@stable
with:
toolchain: ${{ matrix.rust }}
targets: ${{ matrix.target }}
- name: Tests
run: |
cargo rustc "--target=${{ matrix.target }}" --no-default-features --features portable-atomic-critical-section

tests:
runs-on: ubuntu-latest
strategy:
Expand Down
7 changes: 7 additions & 0 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ matrixmultiply = { version = "0.3.2", default-features = false, features=["cgemm
serde = { version = "1.0", optional = true, default-features = false, features = ["alloc"] }
rawpointer = { version = "0.2" }


Copy link
Member

Choose a reason for hiding this comment

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

Can these dependencies be automatically enabled for the relevant platform? I would feel a lot better about this, also lower complexity, if we just managed it all that way with no user visible impact, no feature enablement needed as well.

It's rare that it can be done that way, but here I think the one Arc is a drop-in for the other, and it should work well (?). Or do you see a problem with that?

And yes, it would be good to test this in CI, at least test building it.

Copy link
Member

Choose a reason for hiding this comment

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

See https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html#platform-specific-dependencies

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue is that the critical-section feature is recommended to be passed by the library, as mentioned by portable-atomic.

It is very strongly discouraged to enable this feature in libraries that depend on portable-atomic. The recommended approach for libraries is to leave it up to the end user whether or not to enable this feature. (However, it may make sense to enable this feature by default for libraries specific to a platform where it is guaranteed to always be sound, for example in a hardware abstraction layer targeting a single-core chip.)

According to the Arc docs, we can detect if the target has access to Arc from the configuration flag target_has_atomic = "ptr"

Note: This type is only available on platforms that support atomic loads and stores of pointers, which includes all platforms that support the std crate but not all those which only support alloc. This may be detected at compile time using #[cfg(target_has_atomic = "ptr")].

[dev-dependencies]
defmac = "0.2"
quickcheck = { version = "1.0", default-features = false }
Expand All @@ -70,8 +71,14 @@ docs = ["approx", "serde", "rayon"]
std = ["num-traits/std", "matrixmultiply/std"]
rayon = ["dep:rayon", "std"]

portable-atomic-critical-section = ["portable-atomic/critical-section"]

matrixmultiply-threading = ["matrixmultiply/threading"]

[target.'cfg(not(target_has_atomic = "ptr"))'.dependencies]
portable-atomic = { version = "1.6.0" }
portable-atomic-util = { version = "0.2.0", features = [ "alloc" ] }

[profile.bench]
debug = true
[profile.dev.package.numeric-tests]
Expand Down
4 changes: 4 additions & 0 deletions README.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,10 @@ your `Cargo.toml`.

- Enable the ``threading`` feature in the matrixmultiply package

- ``portable-atomic-critical-section``

- Whether ``portable-atomic`` should use ``critical-section``

How to use with cargo
---------------------

Expand Down
5 changes: 5 additions & 0 deletions src/data_traits.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,12 @@
#[allow(unused_imports)]
use rawpointer::PointerExt;

#[cfg(target_has_atomic = "ptr")]
use alloc::sync::Arc;

#[cfg(not(target_has_atomic = "ptr"))]
use portable_atomic_util::Arc;
Copy link
Member

Choose a reason for hiding this comment

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

Does this affect user experience at all, does it change the API?

It's encapsulated right, so almost nothing can change - fortunately - but if I didn't check this then something like Send- or Sync-ness of some types could change or something like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think Send or Sync should be modified, as far as I can tell here is the only possible public APIs that might be impacted

  1. ArcArray
  2. OwnedArcRepr (which according to the code comments should never be used by the user)
/// ArcArray's representation.
///
/// *Don’t use this type directly—use the type alias
/// [`ArcArray`] for the array type!*
#[derive(Debug)]
pub struct OwnedArcRepr<A>(Arc<OwnedRepr<A>>);

ArcArray as far as I can see, never/rarely gives unfettered access to the OwnedRepr which has the Arc, as such I really don't think there is any affect to user experience, other than idiosyncratic different behavior from portable-atomic-util, which is unlikely, but possible.


#[cfg(not(feature = "std"))]
use alloc::vec::Vec;
use std::mem::MaybeUninit;
Expand Down
5 changes: 5 additions & 0 deletions src/impl_arc_array.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,13 @@
// except according to those terms.

use crate::imp_prelude::*;

#[cfg(target_has_atomic = "ptr")]
use alloc::sync::Arc;

#[cfg(not(target_has_atomic = "ptr"))]
use portable_atomic_util::Arc;

/// Methods specific to `ArcArray`.
///
/// ***See also all methods for [`ArrayBase`]***
Expand Down
5 changes: 5 additions & 0 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,12 @@ extern crate cblas_sys;
#[cfg(feature = "docs")]
pub mod doc;

#[cfg(target_has_atomic = "ptr")]
use alloc::sync::Arc;

#[cfg(not(target_has_atomic = "ptr"))]
use portable_atomic_util::Arc;

use std::marker::PhantomData;

pub use crate::dimension::dim::*;
Expand Down