Feat: Integrate miden-gpu crate into prover #1310
Conversation
There was a problem hiding this comment.
Thank you! This looks great! Not a full review yet, but I left a few comments inline.
The main comment is about treating RPX somewhat separately (i.e., not integrating it into the main structure as much). This is because the situation when RPO and RPX co-exist is temporary and eventually we'll leave only one of them.
For now, we can think of RPX as "experimental" which will be used primarily for benchmarking.
There was a problem hiding this comment.
Looks good! Thank you! I left a couple of nit comments inline - but other than these, this is good to go.
Also, could you rebase from the latest next? We just merged a big PR into next, and while there doesn't seem to be any conflicts here, some lint checks may be failing.
prover/src/gpu/metal/tests.rs
Outdated
| #[cfg(test)] | ||
| mod tests { |
There was a problem hiding this comment.
nit: we don't need this internal module here. The #[cfg(test)] can be applied to the module import in prover/src/gpu/metal/mod.rs
prover/src/gpu/metal/mod.rs
Outdated
| //! This module contains GPU acceleration logic for Apple Silicon devices. | ||
| //! For now, the logic is limited to GPU accelerating trace and constraint commitments, | ||
| //! using the RPO 256 or RPX 256 hash functions. | ||
| mod tests; |
There was a problem hiding this comment.
Related to the previous comment: we can apply #[cfg(test)] here.
Also, I would move this to line ~33 (i.e., put this after use statements.
…oice between rpo and rpx
…r for metal execution prover
TheMenko
force-pushed
the
feat-integrate-miden-gpu-crate
branch
from
5e7e6e6
to
414b2da
Compare
|
Rebase done @bobbinth |
This PR removes
ministark-gpu, and instead adds the newmiden-gpucrate, that is based off ofministark-gpu.miden-gpuadds support for the RPX hash.I've also changed the code in a way to be more generic over Hasher when using metal.
The
MetalRpoExecutionProveris now justMetalExecutionProver.There are also some small changes to air and cli to go along with this change.
Note: This PR requires changes to the
cryptocrate. I opened the pull request that addsRpxRandomCoin.