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.

Sign up for GitHub

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

Jump to bottom

feat: bubble up Instruction::Constrains to be applied as early as possible. #4065

Merged
merged 4 commits into from
Jan 17, 2024
Merged

feat: bubble up Instruction::Constrains to be applied as early as possible. #4065

merged 4 commits into from
Jan 17, 2024

Conversation

TomAFrench
Copy link
Member

@TomAFrench TomAFrench commented Jan 17, 2024 •
edited
Loading

Description

Problem*

Resolves

Summary*

This PR adds a very simple pass which moves any Instruction::Constrains to immediately after when all of their inputs are available. This allows the ACIR gen pass to use this information earlier as well as simplifying using these constraints in order to inform constant folding in future.

Additional Context

Pulled out from #4060

Documentation*

Check one:

  • No documentation needed.
  • Documentation included in this PR.
  • [Exceptional Case] 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 GitHub Actions
Copy link
Contributor

github-actions bot commented Jan 17, 2024 •
edited
Loading

Changes to circuit sizes

Generated at commit: 28c5c0601b9bb214e63ca97a7a9f3c0803b7bb39, compared to commit: 555a4a5970737fca1ab9d3b0513c1d1b73418d8e

🧾 Summary (10% most significant diffs)

Program ACIR opcodes (+/-) % Circuit size (+/-) %
to_be_bytes -18 ✅ -21.69% -15 ✅ -8.98%
references -5 ✅ -83.33% -4 ✅ -40.00%
type_aliases -5 ✅ -83.33% -4 ✅ -40.00%
Full diff report 👇
Program ACIR opcodes (+/-) % Circuit size (+/-) %
struct_inputs 28 (+1) +3.70% 2,842 (+1) +0.04%
operator_overloading 339 (-6) -1.74% 8,214 (0) 0.00%
strings 31 (-38) -55.07% 14,373 (0) 0.00%
nested_array_dynamic 4,447 (-3) -0.07% 13,842 (-3) -0.02%
6_array 533 (-2) -0.37% 4,215 (-2) -0.05%
brillig_fns_as_values 25 (-2) -7.41% 3,465 (-2) -0.06%
slice_dynamic_index 2,410 (-6) -0.25% 10,869 (-7) -0.06%
2_div 25 (-2) -7.41% 2,772 (-3) -0.11%
1_mul 10 (-2) -16.67% 2,758 (-3) -0.11%
3_add 10 (-2) -16.67% 2,756 (-3) -0.11%
7_function 277 (-4) -1.42% 3,034 (-5) -0.16%
regression_2660 13 (-5) -27.78% 2,755 (-5) -0.18%
bit_shifts_runtime 780 (-7) -0.89% 3,840 (-7) -0.18%
signed_division 190 (-10) -5.00% 3,649 (-9) -0.25%
signed_arithmetic 190 (-18) -8.65% 2,922 (-17) -0.58%
slices 602 (-31) -4.90% 4,490 (-38) -0.84%
u128 789 (-45) -5.40% 4,605 (-50) -1.07%
to_be_bytes 65 (-18) -21.69% 152 (-15) -8.98%
references 1 (-5) -83.33% 6 (-4) -40.00%
type_aliases 1 (-5) -83.33% 6 (-4) -40.00%

@TomAFrench TomAFrench requested a review from jfecher January 17, 2024 14:45
@TomAFrench TomAFrench changed the title feat: move Instruction::Constrain as close to where it can be processed as possible. feat: bubble up Instruction::Constrains to be applied as early as possible. Jan 17, 2024
@TomAFrench TomAFrench mentioned this pull request Jan 17, 2024
Closed
@vezenovm
Copy link
Contributor

Does this bug still occur with this change? #2645

@TomAFrench
Copy link
Member Author

TomAFrench commented Jan 17, 2024 •
edited
Loading

Does this bug still occur with this change? #2645

It shouldn't, no. This is because the side_effects_enabled_var is used to calculate the values being constrained as shown here. This means that the constraint can't bubble up higher than the relevant Instruction::EnableSideEffects instruction.

In the general case however, I'm working around that by maintaining a separate list of constrained values for each value of side_effects_enabled_var in #4060

@TomAFrench TomAFrench mentioned this pull request Jan 17, 2024
Closed
5 tasks
jfecher
jfecher approved these changes Jan 17, 2024
View reviewed changes
Copy link
Contributor

@jfecher jfecher left a comment

Choose a reason for hiding this comment

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

The code looks good, but can you elaborate more on why we're seeing constraint count reductions in this PR, before the change in constant folding has been made?

@TomAFrench
Copy link
Member Author

TomAFrench commented Jan 17, 2024 •
edited
Loading

can you elaborate more on why we're seeing constraint count reductions in this PR, before the change in constant folding has been made?

Because as well as SSA, ACIR generation can benefit from this change as it will replace the AcirVar which got constrained with a simpler/constant value (see here). We then optimise out a number of constraints due to ACIR discovering that they are satisfied at compile time.

@TomAFrench TomAFrench added this pull request to the merge queue Jan 17, 2024
@TomAFrench TomAFrench removed this pull request from the merge queue due to a manual request Jan 17, 2024
@TomAFrench TomAFrench enabled auto-merge January 17, 2024 18:06
@TomAFrench TomAFrench added this pull request to the merge queue Jan 17, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 17, 2024
@TomAFrench TomAFrench added this pull request to the merge queue Jan 17, 2024
Merged via the queue into master with commit 66f5cdd Jan 17, 2024
30 checks passed
@TomAFrench TomAFrench deleted the tf/bubble-up-constrains branch January 17, 2024 19:01
@kevaundray kevaundray mentioned this pull request Jan 17, 2024
Merged
@TomAFrench TomAFrench mentioned this pull request Jan 17, 2024
Merged
5 tasks
github-merge-queue bot pushed a commit that referenced this pull request Jan 18, 2024
@TomAFrench
fix: hoist constraints on inputs to top of program (#4076)
447aa34 
# Description

## Problem\*

Followup to #4065 

## Summary\*

This PR fixes a small mistake in #4065. I mixed up the front and back of
the vector for the case where we don't find an instruction for which the
constraint's inputs are a result. This results in constraints on the
program inputs not being pushed up to the beginning of the program.

This PR fixes the index to insert these instructions so that we
constrain the inputs first.

## Additional Context



## Documentation\*

Check one:
- [x] No documentation needed.
- [ ] Documentation included in this PR.
- [ ] **[Exceptional Case]** Documentation to be submitted in a separate
PR.

# PR Checklist\*

- [x] I have tested the changes locally.
- [x] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.
github-merge-queue bot pushed a commit that referenced this pull request Jan 22, 2024
@kevaundray @TomAFrench
chore: Release Noir(0.23.0) (#3862)
5be9f9d 
🤖 I have created a release *beep* *boop*
---


<details><summary>0.23.0</summary>

## [0.23.0](v0.22.0...v0.23.0)
(2024-01-22)


### ⚠ BREAKING CHANGES

* Ban nested slices
([#4018](#4018))
* Breaking changes from aztec-packages
([#3955](#3955))
* Rename Arithmetic opcode to AssertZero
([#3840](#3840))
* remove circuit methods from noir_wasm
([#3869](#3869))

### Features

* Add `assert_max_bit_size` method to `Field`
([#4016](#4016))
([bc9a44f](bc9a44f))
* Add `noir-compiler` checks to `aztec_macros`
([#4031](#4031))
([420a5c7](420a5c7))
* Add a `--force` flag to force a full recompile
([#4054](#4054))
([27a8e68](27a8e68))
* Add dependency resolver for `noir_wasm` and implement `FileManager`
for consistency with native interface
([#3891](#3891))
([c29c7d7](c29c7d7))
* Add foreign call support to `noir_codegen` functions
([#3933](#3933))
([e5e52a8](e5e52a8))
* Add MVP `nargo export` command
([#3870](#3870))
([fbb51ed](fbb51ed))
* Add support for codegenning multiple functions which use the same
structs in their interface
([#3868](#3868))
([1dcfcc5](1dcfcc5))
* Added efficient field comparisons for bn254
([#4042](#4042))
([1f9cad0](1f9cad0))
* Assert maximum bit size when creating a U128 from an integer
([#4024](#4024))
([8f9c7e4](8f9c7e4))
* Avoid unnecessary range checks by inspecting instructions for casts
([#4039](#4039))
([378c18e](378c18e))
* Breaking changes from aztec-packages
([#3955](#3955))
([5be049e](5be049e))
* Bubble up `Instruction::Constrain`s to be applied as early as
possible. ([#4065](#4065))
([66f5cdd](66f5cdd))
* Cached LSP parsing
([#4083](#4083))
([b4f724e](b4f724e))
* Comparison for signed integers
([#3873](#3873))
([bcbd49b](bcbd49b))
* Decompose `Instruction::Cast` to have an explicit truncation
instruction ([#3946](#3946))
([35f18ef](35f18ef))
* Decompose `Instruction::Constrain` into multiple more basic
constraints ([#3892](#3892))
([51cf9d3](51cf9d3))
* Docker testing flow
([#3895](#3895))
([179c90d](179c90d))
* Extract parsing to its own pass and do it in parallel
([#4063](#4063))
([569cbbc](569cbbc))
* Implement `Eq` trait on curve points
([#3944](#3944))
([abf751a](abf751a))
* Implement DAP protocol in Nargo
([#3627](#3627))
([13834d4](13834d4))
* Implement generic traits
([#4000](#4000))
([916fd15](916fd15))
* Implement Operator Overloading
([#3931](#3931))
([4b16090](4b16090))
* **lsp:** Cache definitions for goto requests
([#3930](#3930))
([4a2140f](4a2140f))
* **lsp:** Goto global
([#4043](#4043))
([15237b3](15237b3))
* **lsp:** Goto struct member inside Impl method
([#3918](#3918))
([99c2c5a](99c2c5a))
* **lsp:** Goto trait from trait impl
([#3956](#3956))
([eb566e2](eb566e2))
* **lsp:** Goto trait method declaration
([#3991](#3991))
([eb79166](eb79166))
* **lsp:** Goto type alias
([#4061](#4061))
([dc83385](dc83385))
* **lsp:** Goto type definition
([#4029](#4029))
([8bb4ddf](8bb4ddf))
* **lsp:** Re-add code lens feature with improved performance
([#3829](#3829))
([8f5cd6c](8f5cd6c))
* Optimize array ops for arrays of structs
([#4027](#4027))
([c9ec0d8](c9ec0d8))
* Optimize logic gate ACIR-gen
([#3897](#3897))
([926460a](926460a))
* Prefer `AcirContext`-native methods for performing logic operations
([#3898](#3898))
([0ec39b8](0ec39b8))
* Remove range constraints from witnesses which are constrained to be
constants ([#3928](#3928))
([afe9c7a](afe9c7a))
* Remove truncation from brillig casts
([#3997](#3997))
([857ff97](857ff97))
* Remove truncations which can be seen to be noops using type
information ([#3953](#3953))
([cc3c2c2](cc3c2c2))
* Remove unnecessary predicate from `Lt` instruction
([#3922](#3922))
([a63433f](a63433f))
* Simplify chains of casts to be all in terms of the original `ValueId`
([#3984](#3984))
([2384d3e](2384d3e))
* Simplify multiplications by `0` or `1` in ACIR gen
([#3924](#3924))
([e58844d](e58844d))
* Support for u128
([#3913](#3913))
([b4911dc](b4911dc))
* Support printing more types
([#4071](#4071))
([f5c4632](f5c4632))
* Sync `aztec-packages`
([#4011](#4011))
([fee2452](fee2452))
* Sync commits from `aztec-packages`
([#4068](#4068))
([7a8f3a3](7a8f3a3))
* Use singleton `WasmBlackBoxFunctionSolver` in `noir_js`
([#3966](#3966))
([10b28de](10b28de))


### Bug Fixes

* Acir gen doesn't panic on unsupported BB function
([#3866](#3866))
([34fd978](34fd978))
* Allow abi encoding arrays of structs from JS
([#3867](#3867))
([9b713f8](9b713f8))
* Allow abi encoding tuples from JS
([#3894](#3894))
([f7fa181](f7fa181))
* Allow ast when macro errors
([#4005](#4005))
([efccec3](efccec3))
* Allow lsp to run inside of a docker container
([#3876](#3876))
([2529977](2529977))
* Bit-shifts for signed integers
([#3890](#3890))
([6ddd98a](6ddd98a))
* Checks for cyclic dependencies
([#3699](#3699))
([642011a](642011a))
* **debugger:** Crash when stepping through locations spanning multiple
lines ([#3920](#3920))
([223e860](223e860))
* Don't fail if no tests and the user didn't provide a pattern
([#3864](#3864))
([decbd0f](decbd0f))
* Fix advisory issue in cargo-deny
([#4077](#4077))
([19baea0](19baea0))
* Fixing dark mode background on the CTA button
([#3882](#3882))
([57eae42](57eae42))
* Fixup exports from `noir_wasm`
([#4022](#4022))
([358cdd2](358cdd2))
* Handle multiple imports in the same file
([#3903](#3903))
([219423e](219423e))
* Hoist constraints on inputs to top of program
([#4076](#4076))
([447aa34](447aa34))
* Implement missing codegen for `BlackBoxFunc::EcdsaSecp256r1` in
brillig ([#3943](#3943))
([2c5eceb](2c5eceb))
* Improve `nargo test` output
([#3973](#3973))
([3ab5ff4](3ab5ff4))
* Make `constant_to_radix` emit a slice instead of an array
([#4049](#4049))
([5cdb1d0](5cdb1d0))
* Operator overloading & static trait method references resolving to
generic impls ([#3967](#3967))
([f1de8fa](f1de8fa))
* Preserve brillig entrypoint functions without arguments
([#3951](#3951))
([1111465](1111465))
* Prevent `Instruction::Constrain`s for non-primitive types
([#3916](#3916))
([467948f](467948f))
* Remove panic for adding an invalid crate name in wasm compiler
([#3977](#3977))
([7a1baa5](7a1baa5))
* Return error rather instead of panicking on invalid circuit
([#3976](#3976))
([67201bf](67201bf))
* Search all levels of struct nesting before codegenning primitive types
([#3970](#3970))
([13ae014](13ae014))
* Update generics docs to mention we have traits now
([#3980](#3980))
([c2acdf1](c2acdf1))


### Miscellaneous Chores

* Ban nested slices
([#4018](#4018))
([f8a1fb7](f8a1fb7))
* Remove circuit methods from noir_wasm
([#3869](#3869))
([12d884e](12d884e))
* Rename Arithmetic opcode to AssertZero
([#3840](#3840))
([836f171](836f171))
</details>

<details><summary>0.39.0</summary>

## [0.39.0](v0.38.0...v0.39.0)
(2024-01-22)


### ⚠ BREAKING CHANGES

* Breaking changes from aztec-packages
([#3955](#3955))
* Rename Arithmetic opcode to AssertZero
([#3840](#3840))
* Remove unused methods on ACIR opcodes
([#3841](#3841))
* Remove partial backend feature
([#3805](#3805))

### Features

* Aztec-packages
([#3754](#3754))
([c043265](c043265))
* Breaking changes from aztec-packages
([#3955](#3955))
([5be049e](5be049e))
* Remove range constraints from witnesses which are constrained to be
constants ([#3928](#3928))
([afe9c7a](afe9c7a))
* Speed up transformation of debug messages
([#3815](#3815))
([2a8af1e](2a8af1e))
* Sync `aztec-packages`
([#4011](#4011))
([fee2452](fee2452))
* Sync commits from `aztec-packages`
([#4068](#4068))
([7a8f3a3](7a8f3a3))


### Bug Fixes

* Deserialize odd length hex literals
([#3747](#3747))
([4000fb2](4000fb2))
* Return error rather instead of panicking on invalid circuit
([#3976](#3976))
([67201bf](67201bf))


### Miscellaneous Chores

* Remove partial backend feature
([#3805](#3805))
([0383100](0383100))
* Remove unused methods on ACIR opcodes
([#3841](#3841))
([9e5d0e8](9e5d0e8))
* Rename Arithmetic opcode to AssertZero
([#3840](#3840))
([836f171](836f171))
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

---------

Co-authored-by: TomAFrench <tom@tomfren.ch>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jfecher jfecher jfecher approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@TomAFrench @vezenovm @jfecher