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: Try to turn acir lambdas into brillig #7279

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

aakoshh
Copy link
Contributor

@aakoshh aakoshh commented Feb 4, 2025

Description

Problem*

Resolves #7247

Summary*

Draft because it's unclear how to fix the unconstrained flag assigned to lambdas by the Monomorphizer.

The lambda expression itself inherits the flag from where it's defined, rather than how it's going to be used. I changed it so that if it's created in an unsafe block then it becomes unconstrained (which I now understand is not correct), but there are examples such as array::sort test where we create a lambda in a constrained function, pass it to another constrained function, which uses it in its body in an unsafe block to pass it on to an unconstrained function it calls. It could call it itself outside the unsafe block as well, which means a function parameter would potentially need to be both brillig and acir, depending on which block it's called from. Yet, we pass a single function ID in SSA, so how could it be both?

    pub fn sort(self) -> Self {
        self.sort_via(|a, b| a <= b)
    }
    pub fn sort_via<Env>(self, ordering: fn[Env](T, T) -> bool) -> Self {
        unsafe {
            let sorted = quicksort::quicksort(self, ordering);
    ...

pub(crate) unconstrained fn quicksort<T, Env, let N: u32>(
    _arr: [T; N],
    sortfn: fn[Env](T, T) -> bool,
)

I thought about changing it to unconstrained ordering: fn[Env](T, T) -> bool, and then when we call sort_via we could look at its call parameter types to divine that the lambda should be unconstrained, but if we maintain that it can be called in two ways, then I'm not sure how to do it, because the lambda argument is turned into a function before we process the function we pass it to, so the information of how it's used might not be available until later. Also, the user doesn't have to add this at the moment, in which case an acir lambda is created, and in the SSA we are stuck with the situation we wished to avoid, with brillig calling acir.

I also considered flipping the flag in the lambda function when we process the function body of its caller, but there is no 1:1 correspondence between the callee and the lambda, e.g. multiple lambdas can be passed to it, so we would have to somehow remember which lambdas were passed to which functions.

Perhaps if we process the callee function first, we can gather enough information about how the arguments are used, but it sounds like it would flip everything on its head to process dependencies first, rather than queue them for later. Maybe the SSA pass like the runtime separation used to be is the more convenient way to clone lambdas with caller-specific runtimes.

Additional Context

Documentation*

Check one:

  • No documentation needed.
  • Documentation included in this PR.
  • [For Experimental Features] Documentation to be submitted in a separate PR.

PR Checklist*

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

Comment on lines +505 to +507
self.in_unconstrained_function = true;
let res = self.block(block.statements);
self.in_unconstrained_function = was_in_unconstrained_function;
Copy link
Member

Choose a reason for hiding this comment

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

This seems like we're treating the contents of an unsafe block as unconstrained. This isn't the case however as it's still constrained but the only difference is that we can call into unconstrained functions.

@aakoshh aakoshh changed the title Try to turn acir lambdas into brillig fix: Try to turn acir lambdas into brillig Feb 5, 2025
Comment on lines -239 to +242
let target_fns =
signature_to_functions_as_value.get(&dispatch_signature).cloned().unwrap_or_default();
variants.insert((dispatch_signature, caller_runtime), target_fns);
let key = (dispatch_signature, caller_runtime);
let target_fns = signature_to_functions_as_value.get(&key).cloned().unwrap_or_default();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

With this change we end up in a situation where if the lambda is created in constrained code, and subsequently is passed to unconstrained code, then this list is empty and the compiler crashes further down.

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.

Sync temp fix 2: crash inserting acir-only instruction into brillig runtime
2 participants