fix: Monomorphize function values as pairs of (constrained, unconstrained)

[jfecher]

Description

Problem*

Resolves #7289

Summary*

Changes the monomorphizer to represent function values as a pair of a constrained version and unconstrained version of the same function. In the case of closures, the entire closure tuple is duplicated in each element (so you'd have (constrained_closure_tuple, unconstrained_closure_tuple). Then, when the function is later called we index the tuple to extract the correct version based on the current runtime.

When calling a function, if the function called is a name referring to a known function we avoid monomorphizing both versions and monomorphize only the necessary version instead, although this optimization is not necessary for correctness.

Additional Context

This is the monomorphizer-only variant of this change with no corresponding ?unconstrained syntax. If we're alright with the approach here we could merge this first and add ?unconstrained later (if desired) to limit the functions we compile as both constrained & unconstrained to only the necessary ones.

All lambdas are compiled as both constrained and unconstrained which can add a bunch of `"lambda", "lambda", "lambda",..." entries to the compiled artifact.

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.

[github-actions]

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Opcode count'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.10.

Benchmark suite	Current: 7b7e2bc	Previous: b51b616	Ratio
rollup-block-root-empty	88193 opcodes	68110 opcodes	1.29

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

CC: @TomAFrench

[github-actions]

⚠️ 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: adf99d9	Previous: c6835b5	Ratio
test_report_zkpassport_noir-ecdsa_	3 s	1 s	3

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

CC: @TomAFrench

[github-actions]

Changes to number of Brillig opcodes executed

Generated at commit: 4a196da1a066aa3af0e1c48ee717e8d2d53f34c0, compared to commit: 6870579e0aa844371db698cc52ab3cdf91877d2f

🧾 Summary (10% most significant diffs)

Program	Brillig opcodes (+/-)	%
lambda_from_array_inliner_max	+992 ❌	+58.91%
lambda_from_array_inliner_min	+992 ❌	+58.91%
lambda_from_array_inliner_zero	+992 ❌	+58.91%
lambda_from_dynamic_if_inliner_max	+111 ❌	+37.00%

---

Full diff report 👇

Program	Brillig opcodes (+/-)	%
lambda_from_array_inliner_max	2,676 (+992)	+58.91%
lambda_from_array_inliner_min	2,676 (+992)	+58.91%
lambda_from_array_inliner_zero	2,676 (+992)	+58.91%
lambda_from_dynamic_if_inliner_max	411 (+111)	+37.00%
lambda_from_dynamic_if_inliner_zero	464 (+114)	+32.57%
lambda_from_dynamic_if_inliner_min	476 (+114)	+31.49%
higher_order_functions_inliner_min	2,148 (+375)	+21.15%
lambda_from_global_array_inliner_max	55 (+7)	+14.58%
lambda_from_global_array_inliner_min	55 (+7)	+14.58%
lambda_from_global_array_inliner_zero	55 (+7)	+14.58%
higher_order_functions_inliner_zero	1,291 (+126)	+10.82%
regression_11294_inliner_min	1,255 (+50)	+4.15%
array_sort_inliner_min	989 (+26)	+2.70%
slices_inliner_min	3,848 (+98)	+2.61%
hashmap_inliner_min	79,988 (+672)	+0.85%
hashmap_inliner_zero	71,519 (+440)	+0.62%
uhashmap_inliner_zero	164,368 (+584)	+0.36%
uhashmap_inliner_min	174,422 (+596)	+0.34%
debug_logs_inliner_max	5,130 (+2)	+0.04%
debug_logs_inliner_min	5,343 (+2)	+0.04%
debug_logs_inliner_zero	5,343 (+2)	+0.04%
hashmap_inliner_max	56,956 (+3)	+0.01%

[github-actions]

Changes to Brillig bytecode sizes

Generated at commit: 4a196da1a066aa3af0e1c48ee717e8d2d53f34c0, compared to commit: 6870579e0aa844371db698cc52ab3cdf91877d2f

🧾 Summary (10% most significant diffs)

Program	Brillig opcodes (+/-)	%
lambda_from_array_inliner_max	+1,584 ❌	+63.77%
lambda_from_array_inliner_min	+1,584 ❌	+63.77%
lambda_from_array_inliner_zero	+1,584 ❌	+63.77%

---

Full diff report 👇

Program	Brillig opcodes (+/-)	%
lambda_from_array_inliner_max	4,068 (+1,584)	+63.77%
lambda_from_array_inliner_min	4,068 (+1,584)	+63.77%
lambda_from_array_inliner_zero	4,068 (+1,584)	+63.77%
higher_order_functions_inliner_min	1,607 (+518)	+47.57%
lambda_from_dynamic_if_inliner_max	613 (+123)	+25.10%
higher_order_functions_inliner_zero	747 (+117)	+18.57%
lambda_from_dynamic_if_inliner_zero	501 (+65)	+14.91%
lambda_from_dynamic_if_inliner_min	509 (+65)	+14.64%
lambda_from_global_array_inliner_max	67 (+7)	+11.67%
lambda_from_global_array_inliner_min	67 (+7)	+11.67%
lambda_from_global_array_inliner_zero	67 (+7)	+11.67%
array_sort_inliner_min	470 (+25)	+5.62%
hashmap_inliner_min	9,335 (+471)	+5.31%
regression_11294_inliner_min	303 (+15)	+5.21%
uhashmap_inliner_min	7,434 (+290)	+4.06%
slices_inliner_min	2,211 (+77)	+3.61%
uhashmap_inliner_zero	6,948 (+208)	+3.09%
hashmap_inliner_zero	7,977 (+115)	+1.46%
debug_logs_inliner_max	5,118 (+2)	+0.04%
debug_logs_inliner_min	5,319 (+2)	+0.04%
debug_logs_inliner_zero	5,319 (+2)	+0.04%

[github-actions]

Changes to circuit sizes

Generated at commit: 4a196da1a066aa3af0e1c48ee717e8d2d53f34c0, compared to commit: 6870579e0aa844371db698cc52ab3cdf91877d2f

🧾 Summary (10% most significant diffs)

Program	ACIR opcodes (+/-)	%	Circuit size (+/-)	%
lambda_from_global_array	+15 ❌	+166.67%	+2,763 ❌	+9210.00%
function_ref	+17 ❌	+212.50%	+15 ❌	+65.22%

---

Full diff report 👇

Program	ACIR opcodes (+/-)	%	Circuit size (+/-)	%
lambda_from_global_array	24 (+15)	+166.67%	2,793 (+2,763)	+9210.00%
function_ref	25 (+17)	+212.50%	38 (+15)	+65.22%
regression_8662	18 (+12)	+200.00%	31 (+10)	+47.62%
lambda_from_dynamic_if	206 (+102)	+98.08%	2,948 (+95)	+3.33%
lambda_from_array	1,801 (+994)	+123.17%	1,275 (-2,031)	-61.43%

[github-actions]

⚠️ Performance Alert ⚠️

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

Benchmark suite	Current: a3d6a6b	Previous: 6870579	Ratio
sha512-100-bytes	0.137 s	0.101 s	1.36

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

CC: @TomAFrench

[jfecher]

Looks like arb_ast_roundtrip isn't true anymore. It asserts that the Ast is the same before/after monomorphization but after monomorphization we now turn all functions into tuples of (constrained, unconstrained).

I added a hack to avoid showing the second element of this tuple for tuples and for tuple types of functions but am still seeing some differences. Notably some method calls look like function calls now for some reason (unsure where the code for displaying that is) and because my hack arbitrarily displays the first element of the tuple, unconstrained function types are showing as being changed to constrained.

I'm also not sure if there is value to keeping this test though? It may also be more clear if my hack to not display these function tuples is only optional since without them we're not seeing the code that is actually sent to SSA and it is unclear why we see e.g. lambdas compiled as both constrained and unconstrained but one version looks unused.

[aakoshh]

I'm also not sure if there is value to keeping this test though?

This test did uncover discrepancies between the fuzzer and the actual monomorphizer, so if possible I'd try to keep it. Arguably if the monomorphizer creates tuples for lambdas and the fuzzer does not, then that's a pretty significant difference.

If that in itself isn't a problem, then instead of hacking the display, you could just add avoid_lambdas: true to the config in mono.rs, with an explanation of why we are not testing this.

[jfecher]

@aakoshh thanks - I think I was misunderstanding the purpose of the test then. I'll add the avoid_lambdas switch but I'm not sure that is sufficient - would that avoid first-class functions and function types entirely?

[aakoshh]

I just had a look: avoid_lambdas basically says that no function should have a function-type parameter, so there won't be function passed in calls.

I may have misunderstood something when I added function parameters to the fuzzer, but I thought there are no actual lambdas in the monomorphized AST, only first class functions, it's just that some of them have been created from Noir lambdas. So what I did was that as I create the signature of functions that I'll generate the body for, they have a chance to get a function parameter that matches one of the already generated first class function signatures. Then, when we need to generate a call, we can look for values in the global function list or the local parameter list. But there are no lambda "literals" if that makes sense.

[aakoshh]

What the fuzzer relies heavily on though, is the ability to print the monomorphized AST as Noir with the DisplayAstAsNoir construct, so if you have a hack that can display the tuples as they would be written, that would be used to 1) render the code we have to copy into an integration test if it fails and 2) for the comptime tests.

Maybe the lambda signature to display (in types) could be based on the context of the function being printed?

Notably some method calls look like function calls now for some reason (unsure where the code for displaying that is)

Could that be related to this?

noir/compiler/noirc_frontend/src/monomorphization/printer.rs

Line 528 in 12465f3

if self.print_special_call(special, &call.arguments, f)? {

[aakoshh]

I guess it can also be a problem that I choose a signature of specific first class function for a function parameter, say it's unconstrained, there won't be a constraint counterpart to be passed along it. Maybe the fuzzer only implements the spirit of ?unconstrained scenario, where we only have 1 version.

[jfecher]

All lambdas are compiled as both constrained and unconstrained which can add a bunch of `"lambda", "lambda", "lambda",..." entries to the compiled artifact.

@vezenovm do you know if this'd be an issue? E.g. if users may be expecting only certain names in the artifact and now there are a bunch of lambda entries.

[vezenovm]

Yes, each unconstrained function called from ACIR. In the case of an unconstrained lambda called from ACIR I would expect it to be included in the debug artifacts, which is the desired effect. However, having that name us just "lambda" would be confusing as it does not provide much information. This would be the case with any lambda entry point though.

So yes this is what happens, however, I think this was always the case for entry point lambdas and can be made more user friendly in follow-up work (perhaps a start would be just adding unique index suffixes to matching names like in the profiler).

In testing this I think I found something awry though. For example, I can write the following:

fn main(x: u32) {
    let functions: [unconstrained fn(u32) -> u32; 1] = [x_times_2];
    let f = functions[x];
    unsafe {
        assert_eq(f(x), x * 2);
    }
}
unconstrained fn x_times_2(x: u32) -> u32 {
    x * 2
}

This will give me the appropriate function name of x_times_2 and that will be reflected in the compilation artifacts. Here is the output of nargo info:

+--------------------------+----------------------------+----------------------+--------------+-----------------+
| Package                  | Function                   | Expression Width     | ACIR Opcodes | Brillig Opcodes |
+--------------------------+----------------------------+----------------------+--------------+-----------------+
| dual_constrained_lambdas | main                       | Bounded { width: 4 } | 10           | 41              |
+--------------------------+----------------------------+----------------------+--------------+-----------------+
| dual_constrained_lambdas | x_times_2                  | N/A                  | N/A          | 33              |
+--------------------------+----------------------------+----------------------+--------------+-----------------+
| dual_constrained_lambdas | directive_integer_quotient | N/A                  | N/A          | 8               |
+--------------------------+----------------------------+----------------------+--------------+-----------------+

This is expected as x_times_2 is explicitly specified as an unconstrained entry point.

I can then do the following:

fn main(x: u32) {
    let functions = [|x: u32| x * 2];
    let f = functions[x];
    unsafe {
        assert_eq(f(x), x * 2);
    }
}

This will give lambda as a Brillig entry point:

+--------------------------+------------------+----------------------+--------------+-----------------+
| Package                  | Function         | Expression Width     | ACIR Opcodes | Brillig Opcodes |
+--------------------------+------------------+----------------------+--------------+-----------------+
| dual_constrained_lambdas | main             | Bounded { width: 4 } | 17           | 42              |
+--------------------------+------------------+----------------------+--------------+-----------------+
| dual_constrained_lambdas | lambda           | N/A                  | N/A          | 33              |
+--------------------------+------------------+----------------------+--------------+-----------------+
| dual_constrained_lambdas | directive_invert | N/A                  | N/A          | 9               |
+--------------------------+------------------+----------------------+--------------+-----------------+

Even though the lambda is called in an unsafe block there is nothing that indicates this lambda should be Brillig as it was specified in an ACIR runtime and called from an ACIR runtime. Combined with the bug in #9484 (review) the above leads me to think that we are incorrectly marking runtimes.

[vezenovm]

fn main(x: u32) {
    let functions = [|x: u32| x * 2];
    let f = functions[x];
    unsafe {
        assert_eq(f(x), x * 2);
    }
}

This will give lambda as a Brillig entry point:

[info table]

Even though the lambda is called in an unsafe block there is nothing that indicates this lambda should be Brillig as it was specified in an ACIR runtime and called from an ACIR runtime.

To add onto this statement, when compiling the above with nightly I get a single main ACIR entry point as expected:

+--------------------------+----------+----------------------+--------------+-----------------+
| Package                  | Function | Expression Width     | ACIR Opcodes | Brillig Opcodes |
+--------------------------+----------+----------------------+--------------+-----------------+
| dual_constrained_lambdas | main     | Bounded { width: 4 } | 1            | 0               |
+--------------------------+----------+----------------------+--------------+-----------------+

[jfecher]

I can then do the following:

fn main(x: u32) {
    let functions = [|x: u32| x * 2];
    let f = functions[x];
    unsafe {
        assert_eq(f(x), x * 2);
    }
}

This will give lambda as a Brillig entry point:

+--------------------------+------------------+----------------------+--------------+-----------------+
| Package                  | Function         | Expression Width     | ACIR Opcodes | Brillig Opcodes |
+--------------------------+------------------+----------------------+--------------+-----------------+
| dual_constrained_lambdas | main             | Bounded { width: 4 } | 17           | 42              |
+--------------------------+------------------+----------------------+--------------+-----------------+
| dual_constrained_lambdas | lambda           | N/A                  | N/A          | 33              |
+--------------------------+------------------+----------------------+--------------+-----------------+
| dual_constrained_lambdas | directive_invert | N/A                  | N/A          | 9               |
+--------------------------+------------------+----------------------+--------------+-----------------+

Even though the lambda is called in an unsafe block there is nothing that indicates this lambda should be Brillig as it was specified in an ACIR runtime and called from an ACIR runtime. Combined with the bug in #9484 (review) the above leads me to think that we are incorrectly marking runtimes.

I think this is happening since functions are now compiled as a tuple of (constrained, unconstrained) so when you store them in an array, both versions are preserved - hence the regressions specifically in lambda_from_array. Do you think this needs to be addressed before this fix is merged?

[vezenovm]

Do you think this needs to be addressed before this fix is merged?

I was thinking that this is most likely a similar bug to the one that triggered #9484 (review). Yes, we are storing both versions in an array but it looks like we are triggering the one with the Brillig runtime when we should be triggering the one with ACIR runtime.

If the two bugs have different root causes though I don't think we need to wait on the latter bug to get this PR in.

[jfecher]

In the new dual_constrained_lambdas test I added println(force_brillig); right after the force_brillig declaration in foo.

I get this failure:

error: Assertion failed: 'Bit size for lhs 1 does not match op bit size 254'
   ┌─ src/main.nr:28:5
   │
28 │     f()
   │     ---
   │
   = Call stack:
     1. src/main.nr:2:5
     2. src/main.nr:11:5
     3. std/lib.nr:40:9
     4. src/main.nr:28:5

Failed assertion

But only when I execute with nargo execute --force --show-ssa. Without --show-ssa the test passes and prints false appropriately.

So your program was the following?:

fn main() {
    foo(|| std::runtime::is_unconstrained());
}

// #7289 originates from a lambda being used in both acir & brillig
fn foo(is_brillig: fn() -> bool) {
    // If this is true the `--force-brillig` flag was passed
    let force_brillig = std::runtime::is_unconstrained();
    println(force_brillig);

    assert(!acir_function(is_brillig) | force_brillig);

    // safety:
    unsafe {
        assert(brillig_function(is_brillig));
        assert(brillig_function2(is_brillig));
    };
}

fn acir_function(f: fn() -> bool) -> bool {
    f()
}

unconstrained fn brillig_function(f: fn() -> bool) -> bool {
    f()
}

// acir_function should become unconstrained when called in
// an unconstrained context - and this should apply to the lambda
// it takes as an argument as well.
unconstrained fn brillig_function2(f: fn() -> bool) -> bool {
    acir_function(f)
}

I get no error for this program

[vezenovm]

I get no error for this program

Hmm yes. That is what I had. Let me reproduce once more.

[vezenovm]

Hmm yes. That is what I had. Let me reproduce once more.

Ah you need --show-ssa. That program will fail with the error I sent above. In fact, any print will fail. Even a println before the call to foo will fail when executed with --show-ssa.

[vezenovm]

Ah you need --show-ssa. That program will fail with the error I sent above. In fact, any print will fail. Even a println before the call to foo will fail when executed with --show-ssa.

The bug looks to be the same as the cause of #9097 which is resolved by #9365. You can look at the PR description of #9365 as to why this bug occurs. As this bug is already captured (with a potential solution) we can merge this PR.