Skip to content

Conversation

vezenovm
Copy link
Contributor

@vezenovm vezenovm commented Sep 3, 2025

Description

Problem*

Resolves errors in #9412.

As per the #9484 description we duplicate both constrained and unconstrained versions of closures. Later when the function is called we extract the correct version to call based off of the caller runtime. However, this means we may still have closure variants with differing runtimes for which defunctionalization will attempt to call.

Summary*

#9412 attempted to work around variants with differing runtimes by adding an extra always failing constrain that are attemping to call a function with a different runtime.

For example this is what was done in #9412 (where f3 is an ACIR function):

brillig(inline_always) fn apply f4 {
  b0(v0: Field, v1: Field):
    v4 = eq v0, Field 1
    jmpif v4 then: b3, else: b2
  b2():
    constrain v0 == Field 3
    constrain Field 0 == Field 1, "Error: Calling constrained function from unconstrained function"
    v11 = call f3(v1) -> Field
    jmp b6(v11)
  b3():
    v6 = call f1(v1) -> Field
    jmp b6(v6)
  b6(v13: Field):
    return v13
}

However, this breaks when we get to inlining as we will attempt to inline f3.

We can achieve the same effect by simply not skipping all functions with a differing runtime. All conditionals and the final dispatch constrain will only reference the function IDs for the variants with matching runtimes. Thus, if the frontend generates bad code and attempts to call the ACIR variant, it will be caught by the constrain in the final dispatch. You can see the tests in this PR for a more in depth example. The apply function above will now be the following:

        brillig(inline_always) fn apply f4 {
          b0(v0: Field, v1: Field):
            constrain v0 == Field 1
            v4 = call f1(v1) -> Field
            return v4
        }

I decided to perform the filtering during create_apply_functions so that we can add an extra check for whether we ended up with no variants after filtering against ACIR runtimes. This means we have a frontend bug for closure runtime duplication.

Additional Context

Another option would be to keep what was done in #9412 but mark ACIR -> Brillig calls as never being allowed to be inlined. However, this ACIR being called from Brillig would then be a well-formed SSA and it would have to be protected by constrain instruction as done for the apply method.

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.

@vezenovm vezenovm changed the title fix(defunctionalization): Acir variant in brillig fix(defunctionalization): ACIR variant in Brillig Sep 3, 2025
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Test Suite Duration'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite Current: 1917488 Previous: 8d5ae13 Ratio
test_report_noir-lang_sha512_ 21 s 12 s 1.75

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench

@vezenovm vezenovm requested review from guipublic and a team September 3, 2025 20:25
@vezenovm
Copy link
Contributor Author

vezenovm commented Sep 3, 2025

All external tests from #9412 are now passing. Any failures are due to no report artifacts existing on #9412 due to its failures.

@vezenovm vezenovm merged commit 09782db into gd/issue_9390 Sep 4, 2025
115 of 118 checks passed
@vezenovm vezenovm deleted the mv/acir-variant-in-brillig branch September 4, 2025 13:44
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.

1 participant