Patch plonky3 to use Powdr accelerated Poseidon2 #2077
Conversation
|
we should to extract some stuff from |
lvella
force-pushed
the
patch-recursive-poseidon2
branch
from
86e0792
to
d20ac5b
Compare
lvella
force-pushed
the
patch-recursive-poseidon2
branch
from
d20ac5b
to
6a475ce
Compare
|
|
||
| #[cfg(all(target_os = "zkvm", target_arch = "riscv32"))] | ||
| mod powdr_accel_impl; | ||
| #[cfg(all(target_os = "zkvm", target_arch = "riscv32"))] |
There was a problem hiding this comment.
I think we should also add a feature powdr to the crate and these checks, in case we want to run inside powdr but with software impl. Wdyt?
There was a problem hiding this comment.
I prefer with a powdr feature, but I did the way you told me to do.
There was a problem hiding this comment.
maybe we should just use the 3? I mean, the Cargo.toml powdr-riscv-runtime dep needs the target anyway, we could be restrictive here too
There was a problem hiding this comment.
3 what? 3 conditions in the all?
There was a problem hiding this comment.
yea, the current 2 in the PR plus the feature
There was a problem hiding this comment.
I don't see it would be better than just using the feature.
If compiling to powdr, both with and without the feature would be valid.
But if not, the feature should not be used and the user should get an error if attempted.
There was a problem hiding this comment.
Exactly. I'm fine with only using the feature, but then we allow the user to try to compile to native using the feature, which will fail ofc but will be ugly. If we use the target as well the user actively needs to be using powdr to go that path
There was a problem hiding this comment.
Imagine the user builds for x86-64 and sets the feature. What do you want to happen?
I want a compilation error, and that is what will happen if we only the use the feature.
If we use the 3 conditions, the path is not taken and the feature is silently ignored.
There was a problem hiding this comment.
Ok, this is not possible unless we accept to break --all-features, which I think we don't. The fact that the feature could not be enabled at x86-64 broke our CI, because we build powdr with --all-features. I changed for the feature to be silently ignored.
|
Something between p2-recursion-leo and main broke this build. |
|
Ah, it is the powdr-executor-utils split. Maybe it is best to not merge this into |
|
lint fails |
| @@ -22,6 +22,8 @@ nightly-features = [ | |||
| "p3-mersenne-31/nightly-features", | |||
| "p3-poseidon2/nightly-features", | |||
| ] | |||
| # As a guest in powdr, use accelerated machines. | |||
| powdr-accel = [] | |||
There was a problem hiding this comment.
powdr-accel sounds a bit weird, why not just powdr?
There was a problem hiding this comment.
Because the thing is accelerated with powdr.
I think a feature named just powdr in a library of the powdr project, in a repo named powdr, is confusing.
If not powdr-accel, maybe guest-in-powdr or something like it?
| @@ -67,3 +69,12 @@ serde = { version = "1.0", default-features = false, features = [ | |||
| "alloc", | |||
| ] } | |||
|
|
|||
| [target.'cfg(all(target_os = "zkvm", target_arch = "riscv32"))'.dependencies] | |||
| powdr-riscv-runtime = { path = "../riscv-runtime", features = [ | |||
There was a problem hiding this comment.
it would be good if these were also optional and a dependency of the feature
There was a problem hiding this comment.
Then the feature would be required to build for powdr target.
This way, the feature is just about the accelerated operations, and the target itself "just works".
There was a problem hiding this comment.
ah yea true, this is still imported by the guest's main or something closer to it which will have the import itself
No description provided.