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

rv32im circuit updates #1690

Merged
merged 11 commits into from
Apr 23, 2024
Merged

rv32im circuit updates #1690

merged 11 commits into from
Apr 23, 2024

Conversation

flaub
Copy link
Member

@flaub flaub commented Apr 20, 2024

  • Replace NVCC_PREPEND_FLAGS and NVCC_APPEND_FLAGS with RISC0_NVCC_FLAGS. If RISC0_NVCC_FLAGS is not set, then -arch=native is used as a default.
  • Add sys_input which is a syscall that guests can use to access the input digest that is optionally specified for each proof request.
  • Adjust rv32im circuit to avoid need to do PC adjustments for claims.
  • The post state digest is now always Digest::ZERO when the exit code is Halted.
  • Add warmup phase to benchmarks to account for kernel JIT compilation

@flaub flaub self-assigned this Apr 20, 2024
Copy link

vercel bot commented Apr 20, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
docs-website ✅ Ready (Inspect) Visit Preview Apr 23, 2024 10:39pm
reports ✅ Ready (Inspect) Visit Preview Apr 23, 2024 10:39pm
reports-and-benchmarks ✅ Ready (Inspect) Visit Preview Apr 23, 2024 10:39pm
website ✅ Ready (Inspect) Visit Preview Apr 23, 2024 10:39pm

@@ -136,8 +136,7 @@ impl KernelBuild {

fn compile_cuda(&mut self, output: &str) {
println!("cargo:rerun-if-env-changed=RISC0_CUDA_OPT");
println!("cargo:rerun-if-env-changed=NVCC_PREPEND_FLAGS");
println!("cargo:rerun-if-env-changed=NVCC_APPEND_FLAGS");
println!("cargo:rerun-if-env-changed=RISC0_NVCC_FLAGS");
Copy link
Member Author

Choose a reason for hiding this comment

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

@mothran @rlukata Note: this was changed so that by default, -arch=native is used. In order to build for a specific architecture, use RISC0_NVCC_FLAGS instead of NVCC_PREPEND_FLAGS or NVCC_APPEND_FLAGS.

Copy link
Contributor

@nategraf nategraf left a comment

Choose a reason for hiding this comment

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

Looks good to me. Some comments provided as suggestions, or considerations.

#[cfg_attr(feature = "export-syscalls", no_mangle)]
pub extern "C" fn sys_input(index: u32) -> u32 {
let t0 = ecall::INPUT;
let index = index & 0x07;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is it important that this does not panic? My default here would have been to panic is out of range.

Copy link
Member Author

Choose a reason for hiding this comment

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

Panic in this codepath would cause a lot of bloat. Ideally this function could be inlined. Generally I don't like the idea of the low-level sys- functions doing a panic. We should maybe have them return error codes (I'd say Result but it'd be nice for these functions to be callable from C). I also don't know how C will handle panics, so another reason to avoid panic from these functions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, that's helpful context

risc0/zkvm/src/guest/env.rs Outdated Show resolved Hide resolved
Comment on lines -476 to -482
.clone();
post.pc = post
.pc
.checked_sub(WORD_SIZE as u32)
.ok_or(VerificationError::ReceiptFormatError)?;
post.digest()
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Love to see it 🎉

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.

None yet

4 participants