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

[WIP] fast multiplication #67

Closed
wants to merge 1 commit into from
Closed

Conversation

pdogr
Copy link

@pdogr pdogr commented Feb 3, 2022

Fixes #66
This is a draft PR to add Toom-Cook multiplication with 3 evaluation points to improve the complexity of multiplication.

}

(lo, hi)
pub fn mul_wide(&self, rhs: &Self) -> (Self, Self) {
Copy link
Member

Choose a reason for hiding this comment

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

Please keep this function const fn.

You'll need to use loops instead of iterators, but otherwise it shouldn't be too difficult.

Copy link
Author

Choose a reason for hiding this comment

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

Yes I switched to using while loops, but we can't use &mut T in const contexts and split_at, split_at_mut are not const fns. I'll have to redesign the impl if mul_wide is to be a const fn.

Copy link
Member

Choose a reason for hiding this comment

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

Aah yeah, the use of split_at/split_at_mut is problematic for const fn.

The closest thing that's stable with const fn is probably subslice patterns, but I'm not aware of anything which would provide a proper analogue for split_at/split_at_mut.

Copy link
Member

Choose a reason for hiding this comment

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

One way to move this forward would be to keep the current schoolbook multiplication method for const fn mul_wide, and then use the fast implementation in the Mul traits.

Choose a reason for hiding this comment

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

Apparently there's a konst crate which implements split_at_mut as a const fn.
IIUC, for it to be constant-time it needs rust version 1.64 at minimum, and it's using some unsafe code.
However, it seems like this crate's MSRV is 1.65, so perhaps that'd work?

Copy link
Member

Choose a reason for hiding this comment

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

Would prefer not to add additional dependencies. Curious how it works though... I thought all mut references were still unstable in const fn

Choose a reason for hiding this comment

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

Regarding &mut T in const fns it seems like this is blocking that: rust-lang/rust#57349, although I may be wrong, I feel out of my depth here.

Copy link
Member

@tarcieri tarcieri Jul 10, 2023

Choose a reason for hiding this comment

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

Yeah, I'm confused how the implementation in the konst crate could work on stable. It uses from_raw_parts_mut which is marked const: unstable, and the function doesn't compile on 1.70.0: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=79b84c558fb0ada4fa9e6980a797aa93

error[E0658]: mutable references are not allowed in constant functions

@tarcieri
Copy link
Member

tarcieri commented Dec 7, 2023

Closing in favor of #402 which implements Karatsuba, which seems more appropriate for our applications.

master has diverged significantly anyway in regard to multiplication as we have added more functionality.

@tarcieri tarcieri closed this Dec 7, 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.

Improve multiplication
3 participants