M-Labs
/
artiq-zynq
10
1
Fork 19
Code Issues 13 Pull Requests Releases Wiki Activity

kernel/api: add nalgebra::linalg try_invert_to api #307

Merged
sb10q merged 2 commits from :support_nalgebra into master 2024-07-22 11:13:46 +08:00
Conversation 5 Commits 2 Files Changed 3 +149
abdul124 commented 2024-07-18 15:44:08 +08:00
Contributor
Copy Link

Adds support for nalgebra crate. The following features were needed to support nalgebra in non-std mode:

  1. array_methods
  2. const_fn
  3. extended_key_value_attributes

A fork of nalgebra with required attributes is at https://git.m-labs.hk/abdul124/nalgebra/src/branch/artiq-zynq

The PR also adds nalgebra::linalg::try_invert_to function to the kernel/api.rs to perform in-place matrix inversion.

Adds support for `nalgebra` crate. The following features were needed to support `nalgebra` in `non-std` mode: 1. array_methods 2. const_fn 3. extended_key_value_attributes A fork of `nalgebra` with required attributes is at https://git.m-labs.hk/abdul124/nalgebra/src/branch/artiq-zynq The PR also adds `nalgebra::linalg::try_invert_to` function to the kernel/api.rs to perform in-place matrix inversion.
abdul124 added 1 commit 2024-07-18 15:44:08 +08:00
sb10q reviewed 2024-07-18 15:45:23 +08:00
src/libksupport/Cargo.toml.tpl Outdated
@ -34,1 +34,4 @@
libboard_artiq = { path = "../libboard_artiq" }
[dependencies.nalgebra]
git = "https://git.m-labs.hk/abdul124/nalgebra.git"
sb10q commented 2024-07-18 15:45:23 +08:00
Owner
Copy Link

Please create it under M-Labs. Moving it

Please create it under M-Labs. Moving it
sb10q reviewed 2024-07-18 15:46:52 +08:00
src/libksupport/src/kernel/api.rs Outdated
@ -215,6 +232,9 @@ pub fn resolve(required: &[u8]) -> Option<u32> {
api!(__aeabi_memclr4),
api!(__aeabi_memclr),
// linalg
sb10q commented 2024-07-18 15:46:52 +08:00
Owner
Copy Link

This should go at the end, after libm.

This should go at the end, after libm.
sb10q reviewed 2024-07-18 15:47:32 +08:00
src/libksupport/src/kernel/api.rs Outdated
@ -41,0 +54,4 @@
0 // Return 0 to indicate failure
}
}
sb10q commented 2024-07-18 15:47:32 +08:00
Owner
Copy Link

Comments are too verbose and basically unnecessary, the code is quite obvious already.

Comments are too verbose and basically unnecessary, the code is quite obvious already.
sb10q reviewed 2024-07-18 15:47:41 +08:00
src/libksupport/src/kernel/api.rs Outdated
@ -38,6 +40,21 @@ unsafe extern "C" fn rtio_log(fmt: *const c_char, mut args: ...) {
rtio::write_log(buf.as_slice());
}
unsafe extern "C" fn try_invert_to(dim0: i64, dim1: i64, data: *mut f64) -> i8 {
sb10q commented 2024-07-18 15:47:41 +08:00
Owner
Copy Link

What about other functions?

What about other functions?
sb10q commented 2024-07-18 15:49:04 +08:00
Owner
Copy Link

Also this is function is poorly named. Add a linalg_ prefix or something, otherwise you have no idea what it can possibly do just looking at the name.

Also this is function is poorly named. Add a ``linalg_`` prefix or something, otherwise you have no idea what it can possibly do just looking at the name.
sb10q commented 2024-07-18 15:49:18 +08:00
Owner
Copy Link

The C symbols are a flat namespace.

The C symbols are a flat namespace.
abdul124 commented 2024-07-18 16:05:42 +08:00
Author
Contributor
Copy Link

Also this is function is poorly named. Add a linalg_ prefix or something, otherwise you have no idea what it can possibly do just looking at the name.

Updated function name and removed unnecessary comments.

What about other functions?

I am testing the other function (wilkinson_shift) on standalone. Will create another PR once its implementation is completed.

> Also this is function is poorly named. Add a ``linalg_`` prefix or something, otherwise you have no idea what it can possibly do just looking at the name. Updated function name and removed unnecessary comments. > What about other functions? I am testing the other function (`wilkinson_shift`) on standalone. Will create another PR once its implementation is completed.
sb10q reviewed 2024-07-18 16:02:53 +08:00
src/libksupport/src/kernel/api.rs Outdated
@ -17,6 +19,7 @@ extern "C" {
fn vsnprintf_(buffer: *mut c_char, count: size_t, format: *const c_char, va: VaList) -> c_int;
}
#[no_mangle]
sb10q commented 2024-07-18 16:02:53 +08:00
Owner
Copy Link

Does not belong in this PR.
Isn't no_mangle implied by extern C anyway?

Does not belong in this PR. Isn't no_mangle implied by extern C anyway?
abdul124 commented 2024-07-18 16:20:40 +08:00
Author
Contributor
Copy Link

Does not belong in this PR.

Noted. I will remove it from this PR.

Isn't no_mangle implied by extern C anyway?

I don't think no_mangle is implied by extern C as discussed here: https://github.com/rust-lang/rfcs/issues/674

> Does not belong in this PR. Noted. I will remove it from this PR. > Isn't no_mangle implied by extern C anyway? I don't think `no_mangle` is implied by `extern C` as discussed here: https://github.com/rust-lang/rfcs/issues/674
sb10q commented 2024-07-18 16:23:26 +08:00
Owner
Copy Link

OK. Please send a separate PR to add it then.

OK. Please send a separate PR to add it then.
sb10q reviewed 2024-07-18 16:03:34 +08:00
src/libksupport/src/kernel/api.rs Outdated
@ -321,1 +340,4 @@
},
// linalg
api!(try_invert_to = linalg_try_invert_to),
sb10q commented 2024-07-18 16:03:34 +08:00
Owner
Copy Link

linalg_ in both names. Use the short form of the macro.

linalg_ in both names. Use the short form of the macro.
sb10q commented 2024-07-18 16:05:31 +08:00
Owner
Copy Link

Sorry the short form won't work, it's only for external functions written in C.

Sorry the short form won't work, it's only for external functions written in C.
abdul124 force-pushed support_nalgebra from f4246ba3a7 to e34bf4fc36 2024-07-18 16:23:39 +08:00 Compare
sb10q reviewed 2024-07-18 18:54:11 +08:00
src/libksupport/src/kernel/api.rs Outdated
@ -39,2 +41,4 @@
}
unsafe extern "C" fn linalg_try_invert_to(dim0: i64, dim1: i64, data: *mut f64) -> i8 {
// Provide an interface to nalgebra::linalg::try_invert_to
sb10q commented 2024-07-18 18:54:11 +08:00
Owner
Copy Link

Obvious/trivial comment, remove.

Obvious/trivial comment, remove.
sb10q reviewed 2024-07-18 18:54:45 +08:00
src/libksupport/src/kernel/api.rs Outdated
@ -41,0 +47,4 @@
let mut inverted_matrix = DMatrix::<f64>::zeros(dim0 as usize, dim1 as usize);
if linalg::try_invert_to(matrix, &mut inverted_matrix) {
data_slice.copy_from_slice(inverted_matrix.transpose().as_slice());
sb10q commented 2024-07-18 18:54:45 +08:00
Owner
Copy Link

transpose?

transpose?
abdul124 commented 2024-07-18 19:26:13 +08:00
Author
Contributor
Copy Link

The as_slice function on nalgebra Matrix returns a column-major slice. The transpose is done to get the output in a form compatible (row-by-row) with NDValue.

The `as_slice` function on nalgebra Matrix returns a column-major slice. The transpose is done to get the output in a form compatible (row-by-row) with NDValue.
sb10q commented 2024-07-18 19:31:53 +08:00
Owner
Copy Link

This is what should go in the code comments, not obvious stuff.

This is what should go in the code comments, not obvious stuff.
sb10q commented 2024-07-18 18:58:29 +08:00
Owner
Copy Link

If the only change to nalgebra is the addition of #![feature(const_fn, extended_key_value_attributes, array_methods)], can't we just use upstream and some rustc flags to enable those features?

I doubt a patch with only that will be accepted upstream, it seems recent rustc has those features stabilized and they do not need gating.

If the only change to nalgebra is the addition of ``#![feature(const_fn, extended_key_value_attributes, array_methods)]``, can't we just use upstream and some rustc flags to enable those features? I doubt a patch with only that will be accepted upstream, it seems recent rustc has those features stabilized and they do not need gating.
sb10q commented 2024-07-18 19:03:22 +08:00
Owner
Copy Link

Try setting the environment variable RUSTFLAGS="-Z const_fn -Z extended_key_value_attributes -Z array_methods"

Try setting the environment variable ``RUSTFLAGS="-Z const_fn -Z extended_key_value_attributes -Z array_methods"``
abdul124 commented 2024-07-18 19:15:21 +08:00
Author
Contributor
Copy Link

Try setting the environment variable RUSTFLAGS="-Z const_fn -Z extended_key_value_attributes -Z array_methods"

While adding -Zextended_key_value_attributes and -Zarray_methods to the rust flags in src/.cargo/config works, -Zconst_fn is already specified in some of the other libraries imported in zynq. This causes a double declaration error causes compilation to fail.

> Try setting the environment variable ``RUSTFLAGS="-Z const_fn -Z extended_key_value_attributes -Z array_methods"`` While adding `-Zextended_key_value_attributes` and `-Zarray_methods` to the rust flags in `src/.cargo/config` works, `-Zconst_fn` is already specified in some of the other libraries imported in zynq. This causes a double declaration error causes compilation to fail.
sb10q commented 2024-07-18 19:33:01 +08:00
Owner
Copy Link

What libraries and can it be removed from there so we can specify it in cargo?

What libraries and can it be removed from there so we can specify it in cargo?
abdul124 commented 2024-07-19 11:06:31 +08:00
Author
Contributor
Copy Link

What libraries and can it be removed from there so we can specify it in cargo?

Here are the first two libraries that have the feature const_fn enabled:
The libcortex_a9b2b3e5c933/libcortex_a9/src/lib.rs (L4) declares the const_fn flag.
The core_io 0320cb152d/src/lib.rs (L6) crate also already specifies it.

While libcortex_a9 can be changed in the zynq-rs repo, for core_io we will need to fork the library and make changes similar to what we are doing for nalgebra right now.

> What libraries and can it be removed from there so we can specify it in cargo? Here are the first two libraries that have the feature `const_fn` enabled: The `libcortex_a9`https://git.m-labs.hk/M-Labs/zynq-rs/src/commit/b2b3e5c933cbc4b7cb14adde480d7561a3ae71ee/libcortex_a9/src/lib.rs#L4 declares the `const_fn` flag. The `core_io` https://github.com/jethrogb/rust-core_io/blob/0320cb152d11c4c7874bada687c0de9cc47fecd6/src/lib.rs#L6 crate also already specifies it. While `libcortex_a9` can be changed in the zynq-rs repo, for `core_io` we will need to fork the library and make changes similar to what we are doing for `nalgebra` right now.
abdul124 force-pushed support_nalgebra from e34bf4fc36 to 8ad6fc8f41 2024-07-22 10:53:20 +08:00 Compare
sb10q merged commit bab938c563 into master 2024-07-22 11:13:46 +08:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
priority:critical

   
priority:high

   
priority:low

   
priority:medium
No Label
priority:critical
priority:high
priority:low
priority:medium
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: M-Labs/artiq-zynq#307
No description provided.
Delete Branch ":support_nalgebra"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?