Make compatible with thumbv6m-none-eabi #1384
Conversation
|
Burn project's Ndarray backend would very much appreciate if this PR is merged. We have a long outstanding issue: tracel-ai/burn#302 |
|
@bluss, do you have any more concerns about this pull request? If so, I would love to see if I can address them! |
|
Clippy is fixed on current master. I just approved the CI run. Not sure if I can review sensibly. |
|
What issues make it so you might be unable to review sensibly? I can help with hardware if you need it. Additionally, I could write a test to check that it compiles correctly. I did forget to mention, but here are the commands needed to compile for
|
|
@BjornTheProgrammer Main reason is I'm a quite bad maintainer, as you can see, so it takes some time. So always wary to take on more complexity especially in an area that is not something I know about. |
| use alloc::sync::Arc; | ||
|
|
||
| #[cfg(feature = "portable-atomic")] | ||
| use portable_atomic_util::Arc; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
- ArcArray
- 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.
| @@ -45,6 +45,9 @@ 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" } | |||
|
|
|||
| portable-atomic = { version = "1.6.0", optional = true } | |||
| portable-atomic-util = { version = "0.2.0", features = [ "alloc" ], optional = true } | |||
|
|
|||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
There was a problem hiding this comment.
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")].
|
@bluss thank you so much for the review! Don't worry about taking time, we are all busy 😅. I added the GitHub tests, but I don't have an environment to test that these are properly working, so a once-over would be much advised. Additionally I added a cfg flag to enable the dependency as suggested. I made an analysis of the user impact, and there honestly doesn't seem to be much, if any at all. However further testing should be done, just to verify that really is true. Should I run these tests, or do you have any other suggestions/concerns? |
|
I suggest copying this snippet from matrixmultiply and testing it like that. Just using stable, don't need any older version. (The "ensure_no_std/Cargo.toml" in this link also needs to be changed and not copied.) |
|
Changed to that! Should be fixed now. |
|
MSRV changed to 1.64 on master. Maybe that can solve some problems (but it wasn't changed because of this PR). |
bluss
changed the title
|
Updated to reflect the master branch with 1.64 |
|
Burn is looking forward to this feature. It seems it's close =D |
|
Best would be if you could squash together the changes into one commit. The merge queue preserves commits, but here we don't want to save the history. (The merge queue doesn't allow us to choose merge vs squash depending on the PR, that makes me think it's a big drawback of it.) |
Cargo.toml
Outdated
| @@ -70,8 +71,24 @@ docs = ["approx", "serde", "rayon"] | |||
| std = ["num-traits/std", "matrixmultiply/std"] | |||
| rayon = ["dep:rayon", "std"] | |||
|
|
|||
| critical-section = ["portable-atomic/critical-section"] | |||
There was a problem hiding this comment.
is critical section the right feature name for us? Maybe not
There was a problem hiding this comment.
What would recommend the name to be? Something like no-std-enable-critical-section?
Heepless, a very popular embedded rust project names the feature portable-atomic-critical-section
# Enable polyfilling of atomics via portable-atomic, using critical section for locking
portable-atomic-critical-section = ["dep:portable-atomic", "portable-atomic", "portable-atomic?/critical-section"]There was a problem hiding this comment.
portable-atomic-critical-section sounds good then
BjornTheProgrammer
force-pushed
the
master
branch
from
018c13b
to
015e99e
Compare
|
There I think I've fixed the history, just need to rename |
BjornTheProgrammer
force-pushed
the
master
branch
from
015e99e
to
7dd3638
Compare
|
I've tested on actual hardware, and it seems to be working properly! |
|
@bluss I would just like to thank you very much for the time and effort you put into this. Your contributions are extremely valued. Thanks once again! |
|
Great, I'm working on the release #1401 which is overdue (to say the least) |
Changes
Add the crates
portable-atomicandportable-atomic-utilto make the Arc used in the code compatible with thethumbv6m-none-eabitarget. Also note that this change will probably makendarraywork on more targets than just thethumbv6m-none-eabitarget.Considerations
Those two crates are imported from a GitHub revision. It might be best to wait for an official release to crates.io, but I'm not sure if that's important.
This has also been tested and confirmed to work on a Raspberry Pi Pico.