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

Add leading/trailing ones and zero count instructions (part 1) #1176

Merged
merged 2 commits into from
Feb 26, 2024

Conversation

Fumuran
Copy link
Contributor

@Fumuran Fumuran commented Dec 8, 2023

This PR adds 5 new instructions:

  • u32clz: count leading zeros of the u32 value. [n, ...] -> [clz, ...]
  • u32ctz: count trailing zeros of the u32 value. [n, ...] -> [ctz, ...]
  • u32clo: count leading ones of the u32 value. [n, ...] -> [clo, ...]
  • u32cto: count trailing ones of the u32 value. [n, ...] -> [cto, ...]
  • ilog2: calculate the base 2 logarithm of the top stack element, rounded down. [n, ...] -> [ilog2, ...]

All these instructions work with help of the advice provider.
This PR partially implements features mentioned in #1133 issue. In the next PR corresponding procedures will be implemented in the std::math::u64 module.

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.

Thank you! Not a review yet - but I left a couple of nits inline which also apply to many other places.

assembly/src/assembler/instruction/mod.rs Outdated Show resolved Hide resolved
assembly/src/ast/nodes/mod.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@plafer plafer 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! Left some suggestions and questions

assembly/src/assembler/instruction/u32_ops.rs Outdated Show resolved Hide resolved
advice_provider: &mut A,
process: &S,
) -> Result<HostResponse, ExecutionError> {
let n = process.get_stack_item(0).as_int() as u32;
Copy link
Contributor

Choose a reason for hiding this comment

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

I would change as u32 to try_into()?, effectively returning a meaningful error to the user right away as opposed to silently computing the number of leading zeros on a different number (whatever it wrapped down to).

Same comment applies to other advice injectors

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, but also I think we should rename the injectors to make it clear that these injectors are only for u32 values. The names could be: U32Clz, U32Ctz etc.

Though, one issue with this naming is that it is not consistent with DivU64. So, we either rename it to U64Div or use ClzU32 etc. I slightly prefer the U64Div etc. approach.

assembly/src/assembler/instruction/u32_ops.rs Show resolved Hide resolved
Comment on lines 331 to 334
let n = process.get_stack_item(0).as_int() as u32;
let trailing_zeros = Felt::new(n.trailing_zeros().into());
advice_provider.push_stack(AdviceSource::Value(trailing_zeros))?;
Ok(HostResponse::None)
Copy link
Contributor

Choose a reason for hiding this comment

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

This logic is copy/pasted across the other functions. There are a few ways to extract that logic to make it more generally useful, but one option would be something like

pub(crate) fn push_trailing_zeros<S: ProcessState, A: AdviceProvider>(
    advice_provider: &mut A,
    process: &S,
) -> Result<HostResponse, ExecutionError> {
    push_transformed_stack_top(|stack_top| {
        let stack_top = stack_top.as_int() as u32;
        Felt::new(stack_top.trailing_zeros().into())
    })
}

fn push_transformed_stack_top(f: impl FnOnce(Felt) -> Felt) -> Result<HostResponse, ExecutionError> {
    let stack_top = process.get_stack_item(0);
    let transformed_stack_top = f(stack_top);
    advice_provider.push_stack(AdviceSource::Value(transformed_stack_top))?;
    Ok(HostResponse::None)
}

If we also apply the previous comment about returning an error instead of using as u32, we make the closure return a Result<Felt, ExecutionError> instead of Felt

@Fumuran Fumuran force-pushed the andrew-leading-trailing-one-zero branch 2 times, most recently from b9f1f42 to d6b7450 Compare February 17, 2024 19:41
@Fumuran Fumuran requested a review from plafer February 17, 2024 19:48
@Fumuran Fumuran force-pushed the andrew-leading-trailing-one-zero branch from d6b7450 to 0e7b44b Compare February 18, 2024 09:41
Copy link
Contributor

@plafer plafer left a comment

Choose a reason for hiding this comment

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

LGTM, great work!

The CI failure is due to a recent update to nightly Rust clippy; it's probably best to fix in another PR

Copy link
Contributor

@hackaugusto hackaugusto left a comment

Choose a reason for hiding this comment

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

I think there is a bug in the ilog2 check. (Edit: ahhhh, where is my comment? Adding it 😬 )

@hackaugusto hackaugusto dismissed their stale review February 21, 2024 11:55

The issue I raised was a non-problem

@Fumuran Fumuran force-pushed the andrew-leading-trailing-one-zero branch 2 times, most recently from ea080b2 to 6fd200e Compare February 22, 2024 08:09
@Fumuran Fumuran force-pushed the andrew-leading-trailing-one-zero branch from 6fd200e to 9b75f1d Compare February 23, 2024 07:16
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!

@bobbinth bobbinth merged commit 43352cb into next Feb 26, 2024
15 checks passed
@bobbinth bobbinth deleted the andrew-leading-trailing-one-zero branch February 26, 2024 22:14
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.

4 participants