fix(frontend): Always auto generate foreign function wrappers#10351
fix(frontend): Always auto generate foreign function wrappers#10351
Conversation
There was a problem hiding this comment.
⚠️ 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: c486b33 | Previous: 132eaee | Ratio |
|---|---|---|---|
test_report_zkpassport_noir-ecdsa_ |
3 s |
2 s |
1.50 |
test_report_zkpassport_noir_rsa_ |
2 s |
1 s |
2 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
There was a problem hiding this comment.
⚠️ 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: c486b33 | Previous: 132eaee | Ratio |
|---|---|---|---|
rollup-block-root-single-tx |
0.003 s |
0.002 s |
1.50 |
rollup-root |
0.005 s |
0.004 s |
1.25 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
There was a problem hiding this comment.
⚠️ 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: 392e871 | Previous: c29dd38 | Ratio |
|---|---|---|---|
rollup-block-root-first-empty-tx |
1348 opcodes |
1100 opcodes |
1.23 |
rollup-block-root-single-tx |
1033 opcodes |
876 opcodes |
1.18 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
|
I wonder if extending this to built-ins and intrinsics will have an effect on the AST fuzzer workflow, where we generate random AST, print it as Noir, then parse it back to execute it as comptime code, and just in the assumption that we can print the AST as executable Noir, and put it in integration tests to achieve the same results as the test. For example in a normal Noir program, any built-in function I call will be wrapped in a proxy which the fuzzer does not generate: if I print the AST and parse it, the AST will have the proxies that the original didn't have, which will make the test that I have for an "AST roundtrip" to gauge the fidelity have to avoid using any intrinsic or built-in function at the least. Hopefully the comptime result will be the same, it's just going to potentially make it more difficult to debug any differences if |
| // The actual display of functions only shows the type, but expects the ID. | ||
| // Send a hash so we can interpret the Initial SSA until we wrap these values with a normal function. | ||
| let hash = rustc_hash::FxBuildHasher.hash_one(x); | ||
| fields.push(FieldElement::from(hash)); | ||
| } | ||
| Value::Intrinsic(x) => { | ||
| // Same as foreign functions: just pass something so we can handle the initial SSA even if somehow an intrinsic makes it here. | ||
| let hash = rustc_hash::FxBuildHasher.hash_one(x); | ||
| fields.push(FieldElement::from(hash)); |
There was a problem hiding this comment.
Which situations are these needed?
There was a problem hiding this comment.
This shouldn't be needed after function values have been wrapped. Before they were, trying to print a builtin function caused a panic, as there was no defined way of passing them to a foreign call as a Field, which is the expected value for a function pointer.
|
This PR now includes a small additional change added in 3ceed01 to stop a confusing error difference I was getting because I had |
|
Just a heads up I also tweaked it in 6812e55 |
|
Going to shelve this work. I've been working on it a bit too much, and with @aakoshh's changes already in, there's little reason to continue this over getting started on our growing backlog (and other greenlighting concerns with the non-proxy portions of the monomorphizer & ownership passes). |
Description
Problem*
Alternative to #10160
Resolves #9671 (once the oracle called from constrained function lint is removed)
Summary*
Before defining function metas, we essentially "collect" another function which wraps oracle functions. I did this in a quick and dirty way so it most likely can be cleaned up and it would need to be modified to handle intrinsics as well (or we will need an entirely separate strategy for wrapping those function kinds).
For the
regression_10156test, when we only attempt to print the oracle function pointer like in #10156 this branch succeeds as expected (prints(<<fn() -> Field>>, <<unconstrained fn() -> Field>>)).Additional Context
Documentation*
Check one:
PR Checklist*
cargo fmton default settings.