-
Notifications
You must be signed in to change notification settings - Fork 173
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
CUDA acceleration for trace and constraint commitments #1617
base: next
Are you sure you want to change the base?
Conversation
pollster = { version = "0.4", optional = true } | ||
|
||
[target.'cfg(target_arch = "x86_64")'.dependencies] | ||
miden-gpu = { path = "../../miden-gpu", optional = true } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This and line 33 need to be changed to a publicly released version.
ProvingOptions::RECURSIVE_96_BITS.with_partitions(3, 8), | ||
ProvingOptions::RECURSIVE_128_BITS.with_partitions(3, 8), | ||
ProvingOptions::RECURSIVE_96_BITS.with_partitions(4, 8), | ||
ProvingOptions::RECURSIVE_128_BITS.with_partitions(4, 8), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please double check if these make sense to you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes - these make sense. I might've even removed the options with 3 partitions, but I guess no harm for keeping them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the option with 3 partitions.
My intuition is that it's better to add it if somebody needs it (and can ensure that it works), rather than have an option that may or may not be used and therefore may or may not work in a future release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Thank you! I left some comments inline - they are all pretty minor. Once the next version of miden-gpu
is released, we'll be able to merge this PR.
ProvingOptions::RECURSIVE_96_BITS.with_partitions(3, 8), | ||
ProvingOptions::RECURSIVE_128_BITS.with_partitions(3, 8), | ||
ProvingOptions::RECURSIVE_96_BITS.with_partitions(4, 8), | ||
ProvingOptions::RECURSIVE_128_BITS.with_partitions(4, 8), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes - these make sense. I might've even removed the options with 3 partitions, but I guess no harm for keeping them.
air/src/options.rs
Outdated
/// multiple devices. The number of partitions should be equal to the number of devices. | ||
pub const fn with_partitions(mut self, num_partitions: usize) -> Self { | ||
let hash_rate = match self.hash_fn { | ||
HashFunction::Blake3_192 => 6, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this could be
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to 8. The match
is not really necessary but I kept it so that if HashFunction
s enum ever changes, this code will be revisited.
Integrate CUDA hardware acceleration from miden-gpu. Supersedes #1474 and #1589
Blocked on the release of https://github.com/0xPolygonMiden/miden-gpu/pull/41. Make sure to update Cargo.toml