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

Added log for instruction counts in stwo-adapted-prover #110

Open
wants to merge 1 commit into
base: yuval/adapted_stwo_logs
Choose a base branch
from

Conversation

yuvalsw
Copy link
Collaborator

@yuvalsw yuvalsw commented Oct 7, 2024

This change is Reviewable

@yuvalsw yuvalsw requested a review from Alon-Ti October 7, 2024 16:14
@yuvalsw
Copy link
Collaborator Author

yuvalsw commented Oct 7, 2024

change target branch

@yuvalsw yuvalsw force-pushed the yuval/adapted_stwo_log_cairo_input branch from a088420 to a3107db Compare October 8, 2024 06:55
Copy link
Contributor

@Alon-Ti Alon-Ti left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 4 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @yuvalsw)


stwo_cairo_prover/crates/adapted_prover/src/main.rs line 67 at r2 (raw file):

    let instruction_counts = vm_output.instructions.counts();
    log::info!("Instruction counts: {instruction_counts:?}");

Is it better to impl Display for this so you don't have to use :??


stwo_cairo_prover/crates/prover/src/input/instructions.rs line 27 at r2 (raw file):

///
/// For each opcode with flags, the array describes the different flag combinations. The index
/// refers to the flag combination in little endian. For example, jnz_imm at index 1 (100 in little

bit-reversed

Code quote:

little endian

stwo_cairo_prover/crates/prover/src/input/instructions.rs line 27 at r2 (raw file):

///
/// For each opcode with flags, the array describes the different flag combinations. The index
/// refers to the flag combination in little endian. For example, jnz_imm at index 1 (100 in little

Maybe add a TODO to make this indexing mechanism more explicit in code?


stwo_cairo_prover/crates/prover/src/input/instructions.rs line 29 at r2 (raw file):

/// refers to the flag combination in little endian. For example, jnz_imm at index 1 (100 in little
/// endian) is for: fp (1=true), not taken (0=false), no ap++ (0=false).
/// Note: for the flag "fp/ap", true means fp-based and false means ap-based.

If this is the case you should keep the ap/fp order in the comments.


stwo_cairo_prover/crates/prover/src/input/instructions.rs line 352 at r2 (raw file):

/// The counts of the instructions usage in the input, split to Stwo opcodes.
///
/// For each opcode with flags, the array describes the different flag combinations. The index

Replace repeated comment with "as above" (or, ideally, share the code with the above struct with generics so that neither the comments nor the vector sizes have to be repeated).

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.

2 participants