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

Fix block stack table #1557

Open
wants to merge 1 commit into
base: next
Choose a base branch
from
Open

Fix block stack table #1557

wants to merge 1 commit into from

Conversation

plafer
Copy link
Contributor

@plafer plafer commented Oct 30, 2024

Fixes block stack table (once again).

The problem this time was that the code that removed a row (on RESPAN or END) would always read the IS_CALL and IS_SYSCALL flags, which are only valid on an END operation. The blake3 benchmark happened to have a RESPAN operation with a few RESPANs that had a 1 in one of these rows. The fix is then to completely separate these 2 code paths.

I confirmed that all miden-base test pass.

@plafer plafer force-pushed the plafer-fix-block-stack-table branch from 8c38d46 to b79fd13 Compare October 30, 2024 18:47
Copy link
Contributor

@bobbinth bobbinth 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! Thank you! I left just two doc-related comments inline.

Comment on lines 48 to +49
/// Computes the multiplicand representing the removal of a row from the block stack table.
fn get_block_stack_table_removal_multiplicand<E: FieldElement<BaseField = Felt>>(
fn get_block_stack_table_respan_multiplicand<E: FieldElement<BaseField = Felt>>(
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the comment here needs to be updated.

Comment on lines +67 to +68

fn get_block_stack_table_end_multiplicand<E: FieldElement<BaseField = Felt>>(
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good to add a brief comment here.

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