diff --git a/.release-please-manifest.json b/.release-please-manifest.json index 69b197376db..fe3c2693b52 100644 --- a/.release-please-manifest.json +++ b/.release-please-manifest.json @@ -1,7 +1,7 @@ { - ".": "0.65.1", + ".": "0.65.2", "yarn-project/cli": "0.35.1", - "yarn-project/aztec": "0.65.1", - "barretenberg": "0.65.1", - "barretenberg/ts": "0.65.1" + "yarn-project/aztec": "0.65.2", + "barretenberg": "0.65.2", + "barretenberg/ts": "0.65.2" } diff --git a/CHANGELOG.md b/CHANGELOG.md index 19379f1aa57..76f4e5e34cd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,34 @@ # Changelog +## [0.65.2](https://github.com/AztecProtocol/aztec-packages/compare/aztec-packages-v0.65.1...aztec-packages-v0.65.2) (2024-11-28) + + +### Features + +* Fee foresight support ([#10262](https://github.com/AztecProtocol/aztec-packages/issues/10262)) ([9e19244](https://github.com/AztecProtocol/aztec-packages/commit/9e19244c01440ce7900ba91c0557567e57f017a0)) +* New proving broker ([#10174](https://github.com/AztecProtocol/aztec-packages/issues/10174)) ([6fd5fc1](https://github.com/AztecProtocol/aztec-packages/commit/6fd5fc18bd973b539fb9edfb372181fbe4617f75)) +* Sequential insertion in indexed trees ([#10111](https://github.com/AztecProtocol/aztec-packages/issues/10111)) ([bfd9fa6](https://github.com/AztecProtocol/aztec-packages/commit/bfd9fa68be4147acb3e3feeaf83ed3c9247761be)) +* Swap polys to facilitate dynamic trace overflow ([#9976](https://github.com/AztecProtocol/aztec-packages/issues/9976)) ([b7b282c](https://github.com/AztecProtocol/aztec-packages/commit/b7b282cd0fb306abbe3951a55a1a4f4d42ed7f8e)) + + +### Bug Fixes + +* Don't store indices of zero leaves. ([#10270](https://github.com/AztecProtocol/aztec-packages/issues/10270)) ([c22be8b](https://github.com/AztecProtocol/aztec-packages/commit/c22be8b23e6d16cf4a60509494b979c3edfdba9b)) +* Expect proper duplicate nullifier error patterns in e2e tests ([#10256](https://github.com/AztecProtocol/aztec-packages/issues/10256)) ([4ee8344](https://github.com/AztecProtocol/aztec-packages/commit/4ee83448a24be1944ca8c71d42ae8aa15049af10)) + + +### Miscellaneous + +* Check artifact consistency ([#10271](https://github.com/AztecProtocol/aztec-packages/issues/10271)) ([6a49405](https://github.com/AztecProtocol/aztec-packages/commit/6a494050f85510c18870117f376280d8e10ed486)) +* Dont import things that themselves import jest in imported functions ([#10260](https://github.com/AztecProtocol/aztec-packages/issues/10260)) ([9440c1c](https://github.com/AztecProtocol/aztec-packages/commit/9440c1cf3834eea380014d55eef6e81cff8ffee8)) +* Fix bad merge in integration l1 publisher ([#10272](https://github.com/AztecProtocol/aztec-packages/issues/10272)) ([b5a6aa4](https://github.com/AztecProtocol/aztec-packages/commit/b5a6aa4ce51a27b220162d48ba065a0077b9fcd8)) +* Fixing sol warnings ([#10276](https://github.com/AztecProtocol/aztec-packages/issues/10276)) ([3d113b2](https://github.com/AztecProtocol/aztec-packages/commit/3d113b212b4641b2a97e6b2b0b4835908f3957c8)) +* Pull out sync changes ([#10274](https://github.com/AztecProtocol/aztec-packages/issues/10274)) ([391a6b7](https://github.com/AztecProtocol/aztec-packages/commit/391a6b7377a5253f2c47fa5ec949f255b284da00)) +* Pull value merger code from sync ([#10080](https://github.com/AztecProtocol/aztec-packages/issues/10080)) ([3392629](https://github.com/AztecProtocol/aztec-packages/commit/3392629818e6d51c01ca4c75c1ad916bb4b4fdb1)) +* Remove default gas settings ([#10163](https://github.com/AztecProtocol/aztec-packages/issues/10163)) ([c9a4d88](https://github.com/AztecProtocol/aztec-packages/commit/c9a4d88b15c320e6cc6d79e0721d0f4062d2d840)) +* Replace relative paths to noir-protocol-circuits ([654d801](https://github.com/AztecProtocol/aztec-packages/commit/654d801dc762ce69589a300ef6a2d8fe590527a8)) +* Teardown context in prover coordination test ([#10257](https://github.com/AztecProtocol/aztec-packages/issues/10257)) ([7ea3888](https://github.com/AztecProtocol/aztec-packages/commit/7ea38887e514a4bbdc7ff847efe19bd2d1b74baf)) + ## [0.65.1](https://github.com/AztecProtocol/aztec-packages/compare/aztec-packages-v0.65.0...aztec-packages-v0.65.1) (2024-11-27) diff --git a/barretenberg/CHANGELOG.md b/barretenberg/CHANGELOG.md index 50162ad8d1a..3a08bec7036 100644 --- a/barretenberg/CHANGELOG.md +++ b/barretenberg/CHANGELOG.md @@ -1,5 +1,23 @@ # Changelog +## [0.65.2](https://github.com/AztecProtocol/aztec-packages/compare/barretenberg-v0.65.1...barretenberg-v0.65.2) (2024-11-28) + + +### Features + +* Sequential insertion in indexed trees ([#10111](https://github.com/AztecProtocol/aztec-packages/issues/10111)) ([bfd9fa6](https://github.com/AztecProtocol/aztec-packages/commit/bfd9fa68be4147acb3e3feeaf83ed3c9247761be)) +* Swap polys to facilitate dynamic trace overflow ([#9976](https://github.com/AztecProtocol/aztec-packages/issues/9976)) ([b7b282c](https://github.com/AztecProtocol/aztec-packages/commit/b7b282cd0fb306abbe3951a55a1a4f4d42ed7f8e)) + + +### Bug Fixes + +* Don't store indices of zero leaves. ([#10270](https://github.com/AztecProtocol/aztec-packages/issues/10270)) ([c22be8b](https://github.com/AztecProtocol/aztec-packages/commit/c22be8b23e6d16cf4a60509494b979c3edfdba9b)) + + +### Miscellaneous + +* Pull value merger code from sync ([#10080](https://github.com/AztecProtocol/aztec-packages/issues/10080)) ([3392629](https://github.com/AztecProtocol/aztec-packages/commit/3392629818e6d51c01ca4c75c1ad916bb4b4fdb1)) + ## [0.65.1](https://github.com/AztecProtocol/aztec-packages/compare/barretenberg-v0.65.0...barretenberg-v0.65.1) (2024-11-27) diff --git a/barretenberg/cpp/CMakeLists.txt b/barretenberg/cpp/CMakeLists.txt index 06a8497d636..443ec3af678 100644 --- a/barretenberg/cpp/CMakeLists.txt +++ b/barretenberg/cpp/CMakeLists.txt @@ -6,7 +6,7 @@ cmake_minimum_required(VERSION 3.24 FATAL_ERROR) project( Barretenberg DESCRIPTION "BN254 elliptic curve library, and PLONK SNARK prover" - VERSION 0.65.1 # x-release-please-version + VERSION 0.65.2 # x-release-please-version LANGUAGES CXX C ) # Insert version into `bb` config file diff --git a/barretenberg/ts/CHANGELOG.md b/barretenberg/ts/CHANGELOG.md index f25e4b03415..33e9682d19c 100644 --- a/barretenberg/ts/CHANGELOG.md +++ b/barretenberg/ts/CHANGELOG.md @@ -1,5 +1,12 @@ # Changelog +## [0.65.2](https://github.com/AztecProtocol/aztec-packages/compare/barretenberg.js-v0.65.1...barretenberg.js-v0.65.2) (2024-11-28) + + +### Miscellaneous + +* **barretenberg.js:** Synchronize aztec-packages versions + ## [0.65.1](https://github.com/AztecProtocol/aztec-packages/compare/barretenberg.js-v0.65.0...barretenberg.js-v0.65.1) (2024-11-27) diff --git a/barretenberg/ts/package.json b/barretenberg/ts/package.json index edc70e8efec..42a97212870 100644 --- a/barretenberg/ts/package.json +++ b/barretenberg/ts/package.json @@ -1,7 +1,7 @@ { "name": "@aztec/bb.js", "packageManager": "yarn@1.22.22", - "version": "0.65.1", + "version": "0.65.2", "homepage": "https://github.com/AztecProtocol/aztec-packages/tree/master/barretenberg/ts", "license": "MIT", "type": "module", diff --git a/l1-contracts/test/Outbox.t.sol b/l1-contracts/test/Outbox.t.sol index 007e8ec01e8..ca9db33eb85 100644 --- a/l1-contracts/test/Outbox.t.sol +++ b/l1-contracts/test/Outbox.t.sol @@ -288,7 +288,7 @@ contract OutboxTest is Test { } } - function testCheckOutOfBoundsStatus(uint256 _blockNumber, uint256 _leafIndex) public { + function testCheckOutOfBoundsStatus(uint256 _blockNumber, uint256 _leafIndex) public view { bool outOfBounds = outbox.hasMessageBeenConsumedAtBlockAndIndex(_blockNumber, _leafIndex); assertFalse(outOfBounds); } diff --git a/l1-contracts/test/decoders/Decoders.t.sol b/l1-contracts/test/decoders/Decoders.t.sol index e940cfb739e..c3d47db7bb8 100644 --- a/l1-contracts/test/decoders/Decoders.t.sol +++ b/l1-contracts/test/decoders/Decoders.t.sol @@ -168,7 +168,7 @@ contract DecodersTest is DecoderBase { // The public inputs are computed based of these values, but not directly part of the decoding per say. } - function testComputeKernelLogsIterationWithoutLogs() public { + function testComputeKernelLogsIterationWithoutLogs() public view { bytes memory kernelLogsLength = hex"00000004"; // 4 bytes containing value 4 bytes memory iterationLogsLength = hex"00000000"; // 4 empty bytes indicating that length of this iteration's logs is 0 bytes memory encodedLogs = abi.encodePacked(kernelLogsLength, iterationLogsLength); @@ -181,7 +181,7 @@ contract DecodersTest is DecoderBase { assertEq(logsHash, bytes32(0), "Incorrect logs hash"); } - function testComputeKernelLogs1Iteration() public { + function testComputeKernelLogs1Iteration() public view { // || K_LOGS_LEN | I1_LOGS_LEN | I1_LOGS || // K_LOGS_LEN = 4 + 8 = 12 (hex"0000000c") // I1_LOGS_LEN = 8 (hex"00000008") @@ -215,7 +215,7 @@ contract DecodersTest is DecoderBase { assertEq(logsHash, referenceLogsHash, "Incorrect logs hash"); } - function testComputeKernelLogs2Iterations() public { + function testComputeKernelLogs2Iterations() public view { // || K_LOGS_LEN | I1_LOGS_LEN | I1_LOGS | I2_LOGS_LEN | I2_LOGS || // K_LOGS_LEN = 4 + 8 + 4 + 20 = 36 (hex"00000024") // I1_LOGS_LEN = 8 (hex"00000008") @@ -265,7 +265,7 @@ contract DecodersTest is DecoderBase { assertEq(logsHash, referenceLogsHashFromIteration2, "Incorrect logs hash"); } - function testComputeKernelLogsMiddleIterationWithoutLogs() public { + function testComputeKernelLogsMiddleIterationWithoutLogs() public view { // || K_LOGS_LEN | I1_LOGS_LEN | I1_LOGS | I2_LOGS_LEN | I2_LOGS | I3_LOGS_LEN | I3_LOGS || // K_LOGS_LEN = 4 + 8 + 4 + 0 + 4 + 20 = 40 (hex"00000028") // I1_LOGS_LEN = 8 (hex"00000008") @@ -323,7 +323,7 @@ contract DecodersTest is DecoderBase { assertEq(logsHash, referenceLogsHashFromIteration3, "Incorrect logs hash"); } - function testComputeTxOutHash() public { + function testComputeTxOutHash() public view { // A tx with no msgs should give an out hash of 0 bytes memory encodedMsgs = abi.encodePacked(hex"00"); bytes32 outHash = txsHelper.computeTxOutHash(encodedMsgs); @@ -338,7 +338,7 @@ contract DecodersTest is DecoderBase { assertEq(outHash, expectedOutHash, "Incorrect tx out hash"); } - function testTxsDecoderCorrectlyComputesNumTxEffectsToPad() public { + function testTxsDecoderCorrectlyComputesNumTxEffectsToPad() public view { // Minimum num txs is 2 so when there are no real txs we need to pad to 2 uint32 numTxEffects = 0; uint32 paddedNumTxEffects = txsHelper.computeNumTxEffectsToPad(numTxEffects); @@ -357,7 +357,7 @@ contract DecodersTest is DecoderBase { assertEq(paddedNumTxEffects, 0, "Incorrect number of tx effects to pad"); } - function testTxsDecoderCorrectlyComputesNumMsgsToPad() public { + function testTxsDecoderCorrectlyComputesNumMsgsToPad() public view { uint32 numMsgs = 0; uint32 numMsgsToPad = txsHelper.computeNumMsgsToPad(numMsgs); assertEq(numMsgsToPad, 1, "Incorrect number of msgs to pad"); diff --git a/l1-contracts/test/governance/registry/getCurrentSnapshotTest.t.sol b/l1-contracts/test/governance/registry/getCurrentSnapshotTest.t.sol index 4e9e1d652db..bd7d752338a 100644 --- a/l1-contracts/test/governance/registry/getCurrentSnapshotTest.t.sol +++ b/l1-contracts/test/governance/registry/getCurrentSnapshotTest.t.sol @@ -6,7 +6,7 @@ import {RegistryBase} from "./Base.t.sol"; import {DataStructures} from "@aztec/governance/libraries/DataStructures.sol"; contract GetCurrentSnapshotTest is RegistryBase { - function test_GivenOneListedRollup() external { + function test_GivenOneListedRollup() external view { // it should return the newest DataStructures.RegistrySnapshot memory snapshot = registry.getCurrentSnapshot(); assertEq(snapshot.blockNumber, block.number); diff --git a/l1-contracts/test/governance/registry/getRollup.t.sol b/l1-contracts/test/governance/registry/getRollup.t.sol index 28d336f32e1..bc95b03465e 100644 --- a/l1-contracts/test/governance/registry/getRollup.t.sol +++ b/l1-contracts/test/governance/registry/getRollup.t.sol @@ -4,7 +4,7 @@ pragma solidity >=0.8.27; import {RegistryBase} from "./Base.t.sol"; contract GetRollupTest is RegistryBase { - function test_GivenOneListedRollup() external { + function test_GivenOneListedRollup() external view { // it should return the newest assertEq(registry.getRollup(), address(0xdead)); } diff --git a/l1-contracts/test/governance/registry/getSnapshot.t.sol b/l1-contracts/test/governance/registry/getSnapshot.t.sol index 2c3ee39e8b7..0cf111ec530 100644 --- a/l1-contracts/test/governance/registry/getSnapshot.t.sol +++ b/l1-contracts/test/governance/registry/getSnapshot.t.sol @@ -10,7 +10,7 @@ contract GetSnapshotTest is RegistryBase { _; } - function test_When_versionExists() external givenMultipleListedRollups { + function test_When_versionExists() external view givenMultipleListedRollups { // it should return the snapshot DataStructures.RegistrySnapshot memory snapshot = registry.getSnapshot(0); @@ -19,7 +19,11 @@ contract GetSnapshotTest is RegistryBase { assertEq(registry.numberOfVersions(), 1); } - function test_When_versionDoesNotExists(uint256 _version) external givenMultipleListedRollups { + function test_When_versionDoesNotExists(uint256 _version) + external + view + givenMultipleListedRollups + { // it should return empty snapshot uint256 version = bound(_version, 1, type(uint256).max); diff --git a/l1-contracts/test/governance/registry/getVersionFor.t.sol b/l1-contracts/test/governance/registry/getVersionFor.t.sol index e9a6a05b0c5..51a05a2f781 100644 --- a/l1-contracts/test/governance/registry/getVersionFor.t.sol +++ b/l1-contracts/test/governance/registry/getVersionFor.t.sol @@ -11,7 +11,7 @@ contract GetVersionForTest is RegistryBase { _; } - function test_When_rollupIs0xdead() external givenNoAdditionalListedRollups { + function test_When_rollupIs0xdead() external view givenNoAdditionalListedRollups { // it should return 0 assertEq(registry.getVersionFor(address(0xdead)), 0); } diff --git a/l1-contracts/test/governance/registry/isRollupRegistered.t.sol b/l1-contracts/test/governance/registry/isRollupRegistered.t.sol index b426dedf039..3f6d76f575d 100644 --- a/l1-contracts/test/governance/registry/isRollupRegistered.t.sol +++ b/l1-contracts/test/governance/registry/isRollupRegistered.t.sol @@ -10,12 +10,12 @@ contract IsRollupRegisteredTest is RegistryBase { _; } - function test_When_rollupIs0xdead() external givenNoAdditionalListedRollups { + function test_When_rollupIs0xdead() external view givenNoAdditionalListedRollups { // it should return true assertTrue(registry.isRollupRegistered(address(0xdead))); } - function test_When_rollupNot0xdead(address _rollup) external givenNoAdditionalListedRollups { + function test_When_rollupNot0xdead(address _rollup) external view givenNoAdditionalListedRollups { // it should return false vm.assume(_rollup != address(0xdead)); assertFalse(registry.isRollupRegistered(_rollup)); diff --git a/l1-contracts/test/merkle/TestUtil.sol b/l1-contracts/test/merkle/TestUtil.sol index 2024b461150..397162b03e4 100644 --- a/l1-contracts/test/merkle/TestUtil.sol +++ b/l1-contracts/test/merkle/TestUtil.sol @@ -34,7 +34,7 @@ contract MerkleTestUtil is Test { return (2 ** height) != originalNumber ? ++height : height; } - function testCalculateTreeHeightFromSize() external { + function testCalculateTreeHeightFromSize() external pure { assertEq(calculateTreeHeightFromSize(0), 1); assertEq(calculateTreeHeightFromSize(1), 1); assertEq(calculateTreeHeightFromSize(2), 1); diff --git a/l1-contracts/test/merkle/UnbalancedMerkle.t.sol b/l1-contracts/test/merkle/UnbalancedMerkle.t.sol index e248f878b5a..44727f03c92 100644 --- a/l1-contracts/test/merkle/UnbalancedMerkle.t.sol +++ b/l1-contracts/test/merkle/UnbalancedMerkle.t.sol @@ -24,7 +24,7 @@ contract UnbalancedMerkleTest is Test { txsHelper = new TxsDecoderHelper(); } - function testDecomp() public { + function testDecomp() public view { // Worst case - max num txs uint32 numTxs = 65535; (uint256 min, uint256 max) = txsHelper.computeMinMaxPathLength(numTxs); @@ -62,7 +62,7 @@ contract UnbalancedMerkleTest is Test { // root // / \ // base base - function testComputeTxsEffectsHash2() public { + function testComputeTxsEffectsHash2() public view { // Generate some base leaves bytes32[] memory baseLeaves = new bytes32[](2); for (uint256 i = 0; i < 2; i++) { @@ -84,7 +84,7 @@ contract UnbalancedMerkleTest is Test { // / \ // base base - function testComputeTxsEffectsHash3() public { + function testComputeTxsEffectsHash3() public view { // Generate some base leaves bytes32[] memory baseLeaves = new bytes32[](3); for (uint256 i = 0; i < 3; i++) { @@ -109,7 +109,7 @@ contract UnbalancedMerkleTest is Test { // merge merge // / \ / \ // base base base base - function testComputeTxsEffectsHash5() public { + function testComputeTxsEffectsHash5() public view { // Generate some base leaves bytes32[] memory baseLeaves = new bytes32[](5); for (uint256 i = 0; i < 5; i++) { @@ -139,7 +139,7 @@ contract UnbalancedMerkleTest is Test { // merge1 merge2 base base // / \ / \ // base base base base - function testComputeTxsEffectsHash6() public { + function testComputeTxsEffectsHash6() public view { // Generate some base leaves bytes32[] memory baseLeaves = new bytes32[](6); for (uint256 i = 0; i < 6; i++) { @@ -171,7 +171,7 @@ contract UnbalancedMerkleTest is Test { // merge1 merge2 merge4 base // / \ / \ / \ // base base base base base base - function testComputeTxsEffectsHash7() public { + function testComputeTxsEffectsHash7() public view { // Generate some base leaves bytes32[] memory baseLeaves = new bytes32[](7); for (uint256 i = 0; i < 6; i++) { diff --git a/l1-contracts/test/sparta/Sampling.t.sol b/l1-contracts/test/sparta/Sampling.t.sol index 013512c877b..f537c96b399 100644 --- a/l1-contracts/test/sparta/Sampling.t.sol +++ b/l1-contracts/test/sparta/Sampling.t.sol @@ -44,7 +44,7 @@ contract Sampler { contract SamplingTest is Test { Sampler sampler = new Sampler(); - function testShuffle() public { + function testShuffle() public view { // Sizes pulled out of thin air uint256 setSize = 1024; uint256 commiteeSize = 32; diff --git a/noir/noir-repo/.github/workflows/test-js-packages.yml b/noir/noir-repo/.github/workflows/test-js-packages.yml index 152d8b1653e..4a5d0b8179b 100644 --- a/noir/noir-repo/.github/workflows/test-js-packages.yml +++ b/noir/noir-repo/.github/workflows/test-js-packages.yml @@ -478,6 +478,9 @@ jobs: - name: Install Foundry uses: foundry-rs/foundry-toolchain@v1.2.0 + with: + version: nightly-8660e5b941fe7f4d67e246cfd3dafea330fb53b1 + - name: Install `bb` run: | diff --git a/noir/noir-repo/compiler/noirc_errors/src/position.rs b/noir/noir-repo/compiler/noirc_errors/src/position.rs index 8131db323b9..c7a64c4f422 100644 --- a/noir/noir-repo/compiler/noirc_errors/src/position.rs +++ b/noir/noir-repo/compiler/noirc_errors/src/position.rs @@ -94,8 +94,10 @@ impl Span { self.start() <= other.start() && self.end() >= other.end() } + /// Returns `true` if any point of `self` intersects a point of `other`. + /// Adjacent spans are considered to intersect (so, for example, `0..1` intersects `1..3`). pub fn intersects(&self, other: &Span) -> bool { - self.end() > other.start() && self.start() < other.end() + self.end() >= other.start() && self.start() <= other.end() } pub fn is_smaller(&self, other: &Span) -> bool { @@ -140,3 +142,37 @@ impl Location { self.file == other.file && self.span.contains(&other.span) } } + +#[cfg(test)] +mod tests { + use crate::Span; + + #[test] + fn test_intersects() { + assert!(Span::from(5..10).intersects(&Span::from(5..10))); + + assert!(Span::from(5..10).intersects(&Span::from(5..5))); + assert!(Span::from(5..5).intersects(&Span::from(5..10))); + + assert!(Span::from(10..10).intersects(&Span::from(5..10))); + assert!(Span::from(5..10).intersects(&Span::from(10..10))); + + assert!(Span::from(5..10).intersects(&Span::from(6..9))); + assert!(Span::from(6..9).intersects(&Span::from(5..10))); + + assert!(Span::from(5..10).intersects(&Span::from(4..11))); + assert!(Span::from(4..11).intersects(&Span::from(5..10))); + + assert!(Span::from(5..10).intersects(&Span::from(4..6))); + assert!(Span::from(4..6).intersects(&Span::from(5..10))); + + assert!(Span::from(5..10).intersects(&Span::from(9..11))); + assert!(Span::from(9..11).intersects(&Span::from(5..10))); + + assert!(!Span::from(5..10).intersects(&Span::from(3..4))); + assert!(!Span::from(3..4).intersects(&Span::from(5..10))); + + assert!(!Span::from(5..10).intersects(&Span::from(11..12))); + assert!(!Span::from(11..12).intersects(&Span::from(5..10))); + } +} diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/acir/mod.rs b/noir/noir-repo/compiler/noirc_evaluator/src/acir/mod.rs index 5c7899b5035..46d0924b322 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/acir/mod.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/acir/mod.rs @@ -24,12 +24,10 @@ mod big_int; mod brillig_directive; mod generated_acir; +use crate::brillig::brillig_gen::gen_brillig_for; use crate::brillig::{ brillig_gen::brillig_fn::FunctionContext as BrilligFunctionContext, - brillig_ir::{ - artifact::{BrilligParameter, GeneratedBrillig}, - BrilligContext, - }, + brillig_ir::artifact::{BrilligParameter, GeneratedBrillig}, Brillig, }; use crate::errors::{InternalError, InternalWarning, RuntimeError, SsaReport}; @@ -518,7 +516,7 @@ impl<'a> Context<'a> { let outputs: Vec = vecmap(main_func.returns(), |result_id| dfg.type_of_value(*result_id).into()); - let code = self.gen_brillig_for(main_func, arguments.clone(), brillig)?; + let code = gen_brillig_for(main_func, arguments.clone(), brillig)?; // We specifically do not attempt execution of the brillig code being generated as this can result in it being // replaced with constraints on witnesses to the program outputs. @@ -878,8 +876,7 @@ impl<'a> Context<'a> { None, )? } else { - let code = - self.gen_brillig_for(func, arguments.clone(), brillig)?; + let code = gen_brillig_for(func, arguments.clone(), brillig)?; let generated_pointer = self.shared_context.new_generated_pointer(); let output_values = self.acir_context.brillig_call( @@ -999,47 +996,6 @@ impl<'a> Context<'a> { .collect() } - fn gen_brillig_for( - &self, - func: &Function, - arguments: Vec, - brillig: &Brillig, - ) -> Result, InternalError> { - // Create the entry point artifact - let mut entry_point = BrilligContext::new_entry_point_artifact( - arguments, - BrilligFunctionContext::return_values(func), - func.id(), - ); - entry_point.name = func.name().to_string(); - - // Link the entry point with all dependencies - while let Some(unresolved_fn_label) = entry_point.first_unresolved_function_call() { - let artifact = &brillig.find_by_label(unresolved_fn_label.clone()); - let artifact = match artifact { - Some(artifact) => artifact, - None => { - return Err(InternalError::General { - message: format!("Cannot find linked fn {unresolved_fn_label}"), - call_stack: CallStack::new(), - }) - } - }; - entry_point.link_with(artifact); - // Insert the range of opcode locations occupied by a procedure - if let Some(procedure_id) = &artifact.procedure { - let num_opcodes = entry_point.byte_code.len(); - let previous_num_opcodes = entry_point.byte_code.len() - artifact.byte_code.len(); - // We subtract one as to keep the range inclusive on both ends - entry_point - .procedure_locations - .insert(procedure_id.clone(), (previous_num_opcodes, num_opcodes - 1)); - } - } - // Generate the final bytecode - Ok(entry_point.finish()) - } - /// Handles an ArrayGet or ArraySet instruction. /// To set an index of the array (and create a new array in doing so), pass Some(value) for /// store_value. To just retrieve an index of the array, pass None for store_value. diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen.rs b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen.rs index 786a03031d6..ca4e783aa93 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen.rs @@ -9,11 +9,17 @@ mod variable_liveness; use acvm::FieldElement; use self::{brillig_block::BrilligBlock, brillig_fn::FunctionContext}; -use super::brillig_ir::{ - artifact::{BrilligArtifact, Label}, - BrilligContext, +use super::{ + brillig_ir::{ + artifact::{BrilligArtifact, BrilligParameter, GeneratedBrillig, Label}, + BrilligContext, + }, + Brillig, +}; +use crate::{ + errors::InternalError, + ssa::ir::{dfg::CallStack, function::Function}, }; -use crate::ssa::ir::function::Function; /// Converting an SSA function into Brillig bytecode. pub(crate) fn convert_ssa_function( @@ -36,3 +42,43 @@ pub(crate) fn convert_ssa_function( artifact.name = func.name().to_string(); artifact } + +pub(crate) fn gen_brillig_for( + func: &Function, + arguments: Vec, + brillig: &Brillig, +) -> Result, InternalError> { + // Create the entry point artifact + let mut entry_point = BrilligContext::new_entry_point_artifact( + arguments, + FunctionContext::return_values(func), + func.id(), + ); + entry_point.name = func.name().to_string(); + + // Link the entry point with all dependencies + while let Some(unresolved_fn_label) = entry_point.first_unresolved_function_call() { + let artifact = &brillig.find_by_label(unresolved_fn_label.clone()); + let artifact = match artifact { + Some(artifact) => artifact, + None => { + return Err(InternalError::General { + message: format!("Cannot find linked fn {unresolved_fn_label}"), + call_stack: CallStack::new(), + }) + } + }; + entry_point.link_with(artifact); + // Insert the range of opcode locations occupied by a procedure + if let Some(procedure_id) = &artifact.procedure { + let num_opcodes = entry_point.byte_code.len(); + let previous_num_opcodes = entry_point.byte_code.len() - artifact.byte_code.len(); + // We subtract one as to keep the range inclusive on both ends + entry_point + .procedure_locations + .insert(procedure_id.clone(), (previous_num_opcodes, num_opcodes - 1)); + } + } + // Generate the final bytecode + Ok(entry_point.finish()) +} diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa.rs index 9e11441caf4..45d10323b06 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa.rs @@ -103,6 +103,7 @@ pub(crate) fn optimize_into_acir( Ssa::evaluate_static_assert_and_assert_constant, "After `static_assert` and `assert_constant`:", )? + .run_pass(Ssa::loop_invariant_code_motion, "After Loop Invariant Code Motion:") .try_run_pass(Ssa::unroll_loops_iteratively, "After Unrolling:")? .run_pass(Ssa::simplify_cfg, "After Simplifying (2nd):") .run_pass(Ssa::flatten_cfg, "After Flattening:") diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/function_inserter.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/function_inserter.rs index 5e133072067..a0c23ad70aa 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/function_inserter.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/function_inserter.rs @@ -25,7 +25,7 @@ pub(crate) struct FunctionInserter<'f> { /// /// This is optional since caching arrays relies on the inserter inserting strictly /// in control-flow order. Otherwise, if arrays later in the program are cached first, - /// they may be refered to by instructions earlier in the program. + /// they may be referred to by instructions earlier in the program. array_cache: Option, /// If this pass is loop unrolling, store the block before the loop to optionally diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction.rs index 3b3fd73a1fd..ba854d6a3c1 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction.rs @@ -16,6 +16,7 @@ use crate::ssa::opt::flatten_cfg::value_merger::ValueMerger; use super::{ basic_block::BasicBlockId, dfg::{CallStack, DataFlowGraph}, + function::Function, map::Id, types::{NumericType, Type}, value::{Value, ValueId}, @@ -366,12 +367,12 @@ impl Instruction { } } - pub(crate) fn can_eliminate_if_unused(&self, dfg: &DataFlowGraph) -> bool { + pub(crate) fn can_eliminate_if_unused(&self, function: &Function) -> bool { use Instruction::*; match self { Binary(binary) => { if matches!(binary.operator, BinaryOp::Div | BinaryOp::Mod) { - if let Some(rhs) = dfg.get_numeric_constant(binary.rhs) { + if let Some(rhs) = function.dfg.get_numeric_constant(binary.rhs) { rhs != FieldElement::zero() } else { false @@ -398,7 +399,7 @@ impl Instruction { | RangeCheck { .. } => false, // Some `Intrinsic`s have side effects so we must check what kind of `Call` this is. - Call { func, .. } => match dfg[*func] { + Call { func, .. } => match function.dfg[*func] { // Explicitly allows removal of unused ec operations, even if they can fail Value::Intrinsic(Intrinsic::BlackBox(BlackBoxFunc::MultiScalarMul)) | Value::Intrinsic(Intrinsic::BlackBox(BlackBoxFunc::EmbeddedCurveAdd)) => true, diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs index 95447f941b0..7e41512fd8f 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs @@ -806,7 +806,8 @@ fn simplify_derive_generators( results.push(is_infinite); } let len = results.len(); - let typ = Type::Array(vec![Type::field()].into(), len); + let typ = + Type::Array(vec![Type::field(), Type::field(), Type::unsigned(1)].into(), len / 3); let result = make_array(dfg, results.into(), typ, block, call_stack); SimplifyResult::SimplifiedTo(result) } else { @@ -816,3 +817,34 @@ fn simplify_derive_generators( unreachable!("Unexpected number of arguments to derive_generators"); } } + +#[cfg(test)] +mod tests { + use crate::ssa::{opt::assert_normalized_ssa_equals, Ssa}; + + #[test] + fn simplify_derive_generators_has_correct_type() { + let src = " + brillig(inline) fn main f0 { + b0(): + v0 = make_array [u8 68, u8 69, u8 70, u8 65, u8 85, u8 76, u8 84, u8 95, u8 68, u8 79, u8 77, u8 65, u8 73, u8 78, u8 95, u8 83, u8 69, u8 80, u8 65, u8 82, u8 65, u8 84, u8 79, u8 82] : [u8; 24] + + // This call was previously incorrectly simplified to something that returned `[Field; 3]` + v2 = call derive_pedersen_generators(v0, u32 0) -> [(Field, Field, u1); 1] + + return v2 + } + "; + let ssa = Ssa::from_str(src).unwrap(); + + let expected = " + brillig(inline) fn main f0 { + b0(): + v15 = make_array [u8 68, u8 69, u8 70, u8 65, u8 85, u8 76, u8 84, u8 95, u8 68, u8 79, u8 77, u8 65, u8 73, u8 78, u8 95, u8 83, u8 69, u8 80, u8 65, u8 82, u8 65, u8 84, u8 79, u8 82] : [u8; 24] + v19 = make_array [Field 3728882899078719075161482178784387565366481897740339799480980287259621149274, Field -9903063709032878667290627648209915537972247634463802596148419711785767431332, u1 0] : [(Field, Field, u1); 1] + return v19 + } + "; + assert_normalized_ssa_equals(ssa, expected); + } +} diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/printer.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/printer.rs index b981b81f365..6bebd21fe61 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/printer.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/printer.rs @@ -272,13 +272,13 @@ fn display_constrain_error( ) -> Result { match error { ConstrainError::StaticString(assert_message_string) => { - writeln!(f, " '{assert_message_string:?}'") + writeln!(f, ", {assert_message_string:?}") } ConstrainError::Dynamic(_, is_string, values) => { if let Some(constant_string) = try_to_extract_string_from_error_payload(*is_string, values, &function.dfg) { - writeln!(f, " '{}'", constant_string) + writeln!(f, ", {constant_string:?}") } else { writeln!(f, ", data {}", value_list(function, values)) } diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs index 9f55e69868c..9ee9a52b5ad 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs @@ -19,7 +19,7 @@ //! //! This is the only pass which removes duplicated pure [`Instruction`]s however and so is needed when //! different blocks are merged, i.e. after the [`flatten_cfg`][super::flatten_cfg] pass. -use std::collections::HashSet; +use std::collections::{HashSet, VecDeque}; use acvm::{acir::AcirField, FieldElement}; use iter_extended::vecmap; @@ -28,6 +28,7 @@ use crate::ssa::{ ir::{ basic_block::BasicBlockId, dfg::{DataFlowGraph, InsertInstructionResult}, + dom::DominatorTree, function::Function, instruction::{Instruction, InstructionId}, types::Type, @@ -67,10 +68,10 @@ impl Function { /// The structure of this pass is simple: /// Go through each block and re-insert all instructions. pub(crate) fn constant_fold(&mut self, use_constraint_info: bool) { - let mut context = Context { use_constraint_info, ..Default::default() }; - context.block_queue.push(self.entry_block()); + let mut context = Context::new(self, use_constraint_info); + context.block_queue.push_back(self.entry_block()); - while let Some(block) = context.block_queue.pop() { + while let Some(block) = context.block_queue.pop_front() { if context.visited_blocks.contains(&block) { continue; } @@ -81,34 +82,62 @@ impl Function { } } -#[derive(Default)] struct Context { use_constraint_info: bool, /// Maps pre-folded ValueIds to the new ValueIds obtained by re-inserting the instruction. visited_blocks: HashSet, - block_queue: Vec, + block_queue: VecDeque, + + /// Contains sets of values which are constrained to be equivalent to each other. + /// + /// The mapping's structure is `side_effects_enabled_var => (constrained_value => [(block, simplified_value)])`. + /// + /// We partition the maps of constrained values according to the side-effects flag at the point + /// at which the values are constrained. This prevents constraints which are only sometimes enforced + /// being used to modify the rest of the program. + /// + /// We also keep track of how a value was simplified to other values per block. That is, + /// a same ValueId could have been simplified to one value in one block and to another value + /// in another block. + constraint_simplification_mappings: + HashMap>>, + + // Cache of instructions without any side-effects along with their outputs. + cached_instruction_results: InstructionResultCache, + + dom: DominatorTree, } /// HashMap from (Instruction, side_effects_enabled_var) to the results of the instruction. /// Stored as a two-level map to avoid cloning Instructions during the `.get` call. -type InstructionResultCache = HashMap, Vec>>; +/// +/// In addition to each result, the original BasicBlockId is stored as well. This allows us +/// to deduplicate instructions across blocks as long as the new block dominates the original. +type InstructionResultCache = HashMap, ResultCache>>; + +/// Records the results of all duplicate [`Instruction`]s along with the blocks in which they sit. +/// +/// For more information see [`InstructionResultCache`]. +#[derive(Default)] +struct ResultCache { + results: Vec<(BasicBlockId, Vec)>, +} impl Context { + fn new(function: &Function, use_constraint_info: bool) -> Self { + Self { + use_constraint_info, + visited_blocks: Default::default(), + block_queue: Default::default(), + constraint_simplification_mappings: Default::default(), + cached_instruction_results: Default::default(), + dom: DominatorTree::with_function(function), + } + } + fn fold_constants_in_block(&mut self, function: &mut Function, block: BasicBlockId) { let instructions = function.dfg[block].take_instructions(); - // Cache of instructions without any side-effects along with their outputs. - let mut cached_instruction_results = HashMap::default(); - - // Contains sets of values which are constrained to be equivalent to each other. - // - // The mapping's structure is `side_effects_enabled_var => (constrained_value => simplified_value)`. - // - // We partition the maps of constrained values according to the side-effects flag at the point - // at which the values are constrained. This prevents constraints which are only sometimes enforced - // being used to modify the rest of the program. - let mut constraint_simplification_mappings: HashMap> = - HashMap::default(); let mut side_effects_enabled_var = function.dfg.make_constant(FieldElement::one(), Type::bool()); @@ -117,8 +146,6 @@ impl Context { &mut function.dfg, block, instruction_id, - &mut cached_instruction_results, - &mut constraint_simplification_mappings, &mut side_effects_enabled_var, ); } @@ -126,22 +153,26 @@ impl Context { } fn fold_constants_into_instruction( - &self, + &mut self, dfg: &mut DataFlowGraph, block: BasicBlockId, id: InstructionId, - instruction_result_cache: &mut InstructionResultCache, - constraint_simplification_mappings: &mut HashMap>, side_effects_enabled_var: &mut ValueId, ) { let constraint_simplification_mapping = - constraint_simplification_mappings.entry(*side_effects_enabled_var).or_default(); - let instruction = Self::resolve_instruction(id, dfg, constraint_simplification_mapping); + self.constraint_simplification_mappings.get(side_effects_enabled_var); + let instruction = Self::resolve_instruction( + id, + block, + dfg, + &mut self.dom, + constraint_simplification_mapping, + ); let old_results = dfg.instruction_results(id).to_vec(); // If a copy of this instruction exists earlier in the block, then reuse the previous results. if let Some(cached_results) = - Self::get_cached(dfg, instruction_result_cache, &instruction, *side_effects_enabled_var) + self.get_cached(dfg, &instruction, *side_effects_enabled_var, block) { Self::replace_result_ids(dfg, &old_results, cached_results); return; @@ -156,9 +187,8 @@ impl Context { instruction.clone(), new_results, dfg, - instruction_result_cache, - constraint_simplification_mapping, *side_effects_enabled_var, + block, ); // If we just inserted an `Instruction::EnableSideEffectsIf`, we need to update `side_effects_enabled_var` @@ -171,8 +201,10 @@ impl Context { /// Fetches an [`Instruction`] by its [`InstructionId`] and fully resolves its inputs. fn resolve_instruction( instruction_id: InstructionId, + block: BasicBlockId, dfg: &DataFlowGraph, - constraint_simplification_mapping: &HashMap, + dom: &mut DominatorTree, + constraint_simplification_mapping: Option<&HashMap>>, ) -> Instruction { let instruction = dfg[instruction_id].clone(); @@ -183,19 +215,30 @@ impl Context { // constraints to the cache. fn resolve_cache( dfg: &DataFlowGraph, - cache: &HashMap, + dom: &mut DominatorTree, + cache: Option<&HashMap>>, value_id: ValueId, + block: BasicBlockId, ) -> ValueId { let resolved_id = dfg.resolve(value_id); - match cache.get(&resolved_id) { - Some(cached_value) => resolve_cache(dfg, cache, *cached_value), - None => resolved_id, + let Some(cached_values) = cache.and_then(|cache| cache.get(&resolved_id)) else { + return resolved_id; + }; + + for (cached_block, cached_value) in cached_values { + // We can only use the simplified value if it was simplified in a block that dominates the current one + if dom.dominates(*cached_block, block) { + return resolve_cache(dfg, dom, cache, *cached_value, block); + } } + + resolved_id } // Resolve any inputs to ensure that we're comparing like-for-like instructions. - instruction - .map_values(|value_id| resolve_cache(dfg, constraint_simplification_mapping, value_id)) + instruction.map_values(|value_id| { + resolve_cache(dfg, dom, constraint_simplification_mapping, value_id, block) + }) } /// Pushes a new [`Instruction`] into the [`DataFlowGraph`] which applies any optimizations @@ -229,39 +272,23 @@ impl Context { } fn cache_instruction( - &self, + &mut self, instruction: Instruction, instruction_results: Vec, dfg: &DataFlowGraph, - instruction_result_cache: &mut InstructionResultCache, - constraint_simplification_mapping: &mut HashMap, side_effects_enabled_var: ValueId, + block: BasicBlockId, ) { if self.use_constraint_info { // If the instruction was a constraint, then create a link between the two `ValueId`s // to map from the more complex to the simpler value. if let Instruction::Constrain(lhs, rhs, _) = instruction { // These `ValueId`s should be fully resolved now. - match (&dfg[lhs], &dfg[rhs]) { - // Ignore trivial constraints - (Value::NumericConstant { .. }, Value::NumericConstant { .. }) => (), - - // Prefer replacing with constants where possible. - (Value::NumericConstant { .. }, _) => { - constraint_simplification_mapping.insert(rhs, lhs); - } - (_, Value::NumericConstant { .. }) => { - constraint_simplification_mapping.insert(lhs, rhs); - } - // Otherwise prefer block parameters over instruction results. - // This is as block parameters are more likely to be a single witness rather than a full expression. - (Value::Param { .. }, Value::Instruction { .. }) => { - constraint_simplification_mapping.insert(rhs, lhs); - } - (Value::Instruction { .. }, Value::Param { .. }) => { - constraint_simplification_mapping.insert(lhs, rhs); - } - (_, _) => (), + if let Some((complex, simple)) = simplify(dfg, lhs, rhs) { + self.get_constraint_map(side_effects_enabled_var) + .entry(complex) + .or_default() + .push((block, simple)); } } } @@ -273,13 +300,22 @@ impl Context { self.use_constraint_info && instruction.requires_acir_gen_predicate(dfg); let predicate = use_predicate.then_some(side_effects_enabled_var); - instruction_result_cache + self.cached_instruction_results .entry(instruction) .or_default() - .insert(predicate, instruction_results); + .entry(predicate) + .or_default() + .cache(block, instruction_results); } } + fn get_constraint_map( + &mut self, + side_effects_enabled_var: ValueId, + ) -> &mut HashMap> { + self.constraint_simplification_mappings.entry(side_effects_enabled_var).or_default() + } + /// Replaces a set of [`ValueId`]s inside the [`DataFlowGraph`] with another. fn replace_result_ids( dfg: &mut DataFlowGraph, @@ -292,22 +328,59 @@ impl Context { } fn get_cached<'a>( + &'a mut self, dfg: &DataFlowGraph, - instruction_result_cache: &'a mut InstructionResultCache, instruction: &Instruction, side_effects_enabled_var: ValueId, - ) -> Option<&'a Vec> { - let results_for_instruction = instruction_result_cache.get(instruction); + block: BasicBlockId, + ) -> Option<&'a [ValueId]> { + let results_for_instruction = self.cached_instruction_results.get(instruction)?; - // See if there's a cached version with no predicate first - if let Some(results) = results_for_instruction.and_then(|map| map.get(&None)) { - return Some(results); - } + let predicate = self.use_constraint_info && instruction.requires_acir_gen_predicate(dfg); + let predicate = predicate.then_some(side_effects_enabled_var); + + results_for_instruction.get(&predicate)?.get(block, &mut self.dom) + } +} - let predicate = - instruction.requires_acir_gen_predicate(dfg).then_some(side_effects_enabled_var); +impl ResultCache { + /// Records that an `Instruction` in block `block` produced the result values `results`. + fn cache(&mut self, block: BasicBlockId, results: Vec) { + self.results.push((block, results)); + } - results_for_instruction.and_then(|map| map.get(&predicate)) + /// Returns a set of [`ValueId`]s produced from a copy of this [`Instruction`] which sits + /// within a block which dominates `block`. + /// + /// We require that the cached instruction's block dominates `block` in order to avoid + /// cycles causing issues (e.g. two instructions being replaced with the results of each other + /// such that neither instruction exists anymore.) + fn get(&self, block: BasicBlockId, dom: &mut DominatorTree) -> Option<&[ValueId]> { + for (origin_block, results) in &self.results { + if dom.dominates(*origin_block, block) { + return Some(results); + } + } + None + } +} + +/// Check if one expression is simpler than the other. +/// Returns `Some((complex, simple))` if a simplification was found, otherwise `None`. +/// Expects the `ValueId`s to be fully resolved. +fn simplify(dfg: &DataFlowGraph, lhs: ValueId, rhs: ValueId) -> Option<(ValueId, ValueId)> { + match (&dfg[lhs], &dfg[rhs]) { + // Ignore trivial constraints + (Value::NumericConstant { .. }, Value::NumericConstant { .. }) => None, + + // Prefer replacing with constants where possible. + (Value::NumericConstant { .. }, _) => Some((rhs, lhs)), + (_, Value::NumericConstant { .. }) => Some((lhs, rhs)), + // Otherwise prefer block parameters over instruction results. + // This is as block parameters are more likely to be a single witness rather than a full expression. + (Value::Param { .. }, Value::Instruction { .. }) => Some((rhs, lhs)), + (Value::Instruction { .. }, Value::Param { .. }) => Some((lhs, rhs)), + (_, _) => None, } } @@ -673,4 +746,48 @@ mod test { let ending_instruction_count = instructions.len(); assert_eq!(ending_instruction_count, 1); } + + #[test] + fn deduplicate_across_blocks() { + // fn main f0 { + // b0(v0: u1): + // v1 = not v0 + // jmp b1() + // b1(): + // v2 = not v0 + // return v2 + // } + let main_id = Id::test_new(0); + + // Compiling main + let mut builder = FunctionBuilder::new("main".into(), main_id); + let b1 = builder.insert_block(); + + let v0 = builder.add_parameter(Type::bool()); + let _v1 = builder.insert_not(v0); + builder.terminate_with_jmp(b1, Vec::new()); + + builder.switch_to_block(b1); + let v2 = builder.insert_not(v0); + builder.terminate_with_return(vec![v2]); + + let ssa = builder.finish(); + let main = ssa.main(); + assert_eq!(main.dfg[main.entry_block()].instructions().len(), 1); + assert_eq!(main.dfg[b1].instructions().len(), 1); + + // Expected output: + // + // fn main f0 { + // b0(v0: u1): + // v1 = not v0 + // jmp b1() + // b1(): + // return v1 + // } + let ssa = ssa.fold_constants_using_constraints(); + let main = ssa.main(); + assert_eq!(main.dfg[main.entry_block()].instructions().len(), 1); + assert_eq!(main.dfg[b1].instructions().len(), 0); + } } diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/die.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/die.rs index 666a8e32246..8d3fa9cc615 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -172,7 +172,7 @@ impl Context { fn is_unused(&self, instruction_id: InstructionId, function: &Function) -> bool { let instruction = &function.dfg[instruction_id]; - if instruction.can_eliminate_if_unused(&function.dfg) { + if instruction.can_eliminate_if_unused(function) { let results = function.dfg.instruction_results(instruction_id); results.iter().all(|result| !self.used_values.contains(result)) } else if let Instruction::Call { func, arguments } = instruction { diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs index b6899a71fee..5d114672a55 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs @@ -447,6 +447,16 @@ impl<'f> Context<'f> { }; self.condition_stack.push(cond_context); self.insert_current_side_effects_enabled(); + + // We disallow this case as it results in the `else_destination` block + // being inlined before the `then_destination` block due to block deduplication in the work queue. + // + // The `else_destination` block then gets treated as if it were the `then_destination` block + // and has the incorrect condition applied to it. + assert_ne!( + self.branch_ends[if_entry], *then_destination, + "ICE: branches merge inside of `then` branch" + ); vec![self.branch_ends[if_entry], *else_destination, *then_destination] } @@ -1526,4 +1536,23 @@ mod test { _ => unreachable!("Should have terminator instruction"), } } + + #[test] + #[should_panic = "ICE: branches merge inside of `then` branch"] + fn panics_if_branches_merge_within_then_branch() { + //! This is a regression test for https://github.com/noir-lang/noir/issues/6620 + + let src = " + acir(inline) fn main f0 { + b0(v0: u1): + jmpif v0 then: b2, else: b1 + b2(): + return + b1(): + jmp b2() + } + "; + let merged_ssa = Ssa::from_str(src).unwrap(); + let _ = merged_ssa.flatten_cfg(); + } } diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs new file mode 100644 index 00000000000..14233ca73e5 --- /dev/null +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs @@ -0,0 +1,378 @@ +//! The loop invariant code motion pass moves code from inside a loop to before the loop +//! if that code will always have the same result on every iteration of the loop. +//! +//! To identify a loop invariant, check whether all of an instruction's values are: +//! - Outside of the loop +//! - Constant +//! - Already marked as loop invariants +//! +//! We also check that we are not hoisting instructions with side effects. +use fxhash::FxHashSet as HashSet; + +use crate::ssa::{ + ir::{ + basic_block::BasicBlockId, + function::{Function, RuntimeType}, + function_inserter::FunctionInserter, + instruction::InstructionId, + value::ValueId, + }, + Ssa, +}; + +use super::unrolling::{Loop, Loops}; + +impl Ssa { + #[tracing::instrument(level = "trace", skip(self))] + pub(crate) fn loop_invariant_code_motion(mut self) -> Ssa { + let brillig_functions = self + .functions + .iter_mut() + .filter(|(_, func)| matches!(func.runtime(), RuntimeType::Brillig(_))); + + for (_, function) in brillig_functions { + function.loop_invariant_code_motion(); + } + + self + } +} + +impl Function { + fn loop_invariant_code_motion(&mut self) { + Loops::find_all(self).hoist_loop_invariants(self); + } +} + +impl Loops { + fn hoist_loop_invariants(self, function: &mut Function) { + let mut context = LoopInvariantContext::new(function); + + for loop_ in self.yet_to_unroll.iter() { + let Ok(pre_header) = loop_.get_pre_header(context.inserter.function, &self.cfg) else { + // If the loop does not have a preheader we skip hoisting loop invariants for this loop + continue; + }; + context.hoist_loop_invariants(loop_, pre_header); + } + + context.map_dependent_instructions(); + } +} + +struct LoopInvariantContext<'f> { + inserter: FunctionInserter<'f>, + defined_in_loop: HashSet, + loop_invariants: HashSet, +} + +impl<'f> LoopInvariantContext<'f> { + fn new(function: &'f mut Function) -> Self { + Self { + inserter: FunctionInserter::new(function), + defined_in_loop: HashSet::default(), + loop_invariants: HashSet::default(), + } + } + + fn hoist_loop_invariants(&mut self, loop_: &Loop, pre_header: BasicBlockId) { + self.set_values_defined_in_loop(loop_); + + for block in loop_.blocks.iter() { + for instruction_id in self.inserter.function.dfg[*block].take_instructions() { + let hoist_invariant = self.can_hoist_invariant(instruction_id); + + if hoist_invariant { + self.inserter.push_instruction(instruction_id, pre_header); + } else { + self.inserter.push_instruction(instruction_id, *block); + } + + self.update_values_defined_in_loop_and_invariants(instruction_id, hoist_invariant); + } + } + } + + /// Gather the variables declared within the loop + fn set_values_defined_in_loop(&mut self, loop_: &Loop) { + for block in loop_.blocks.iter() { + let params = self.inserter.function.dfg.block_parameters(*block); + self.defined_in_loop.extend(params); + for instruction_id in self.inserter.function.dfg[*block].instructions() { + let results = self.inserter.function.dfg.instruction_results(*instruction_id); + self.defined_in_loop.extend(results); + } + } + } + + /// Update any values defined in the loop and loop invariants after a + /// analyzing and re-inserting a loop's instruction. + fn update_values_defined_in_loop_and_invariants( + &mut self, + instruction_id: InstructionId, + hoist_invariant: bool, + ) { + let results = self.inserter.function.dfg.instruction_results(instruction_id).to_vec(); + // We will have new IDs after pushing instructions. + // We should mark the resolved result IDs as also being defined within the loop. + let results = + results.into_iter().map(|value| self.inserter.resolve(value)).collect::>(); + self.defined_in_loop.extend(results.iter()); + + // We also want the update result IDs when we are marking loop invariants as we may not + // be going through the blocks of the loop in execution order + if hoist_invariant { + // Track already found loop invariants + self.loop_invariants.extend(results.iter()); + } + } + + fn can_hoist_invariant(&mut self, instruction_id: InstructionId) -> bool { + let mut is_loop_invariant = true; + // The list of blocks for a nested loop contain any inner loops as well. + // We may have already re-inserted new instructions if two loops share blocks + // so we need to map all the values in the instruction which we want to check. + let (instruction, _) = self.inserter.map_instruction(instruction_id); + instruction.for_each_value(|value| { + // If an instruction value is defined in the loop and not already a loop invariant + // the instruction results are not loop invariants. + // + // We are implicitly checking whether the values are constant as well. + // The set of values defined in the loop only contains instruction results and block parameters + // which cannot be constants. + is_loop_invariant &= + !self.defined_in_loop.contains(&value) || self.loop_invariants.contains(&value); + }); + is_loop_invariant && instruction.can_be_deduplicated(&self.inserter.function.dfg, false) + } + + fn map_dependent_instructions(&mut self) { + let blocks = self.inserter.function.reachable_blocks(); + for block in blocks { + for instruction_id in self.inserter.function.dfg[block].take_instructions() { + self.inserter.push_instruction(instruction_id, block); + } + self.inserter.map_terminator_in_place(block); + } + } +} + +#[cfg(test)] +mod test { + use crate::ssa::opt::assert_normalized_ssa_equals; + use crate::ssa::Ssa; + + #[test] + fn simple_loop_invariant_code_motion() { + let src = " + brillig(inline) fn main f0 { + b0(v0: u32, v1: u32): + jmp b1(u32 0) + b1(v2: u32): + v5 = lt v2, u32 4 + jmpif v5 then: b3, else: b2 + b3(): + v6 = mul v0, v1 + constrain v6 == u32 6 + v8 = add v2, u32 1 + jmp b1(v8) + b2(): + return + } + "; + + let mut ssa = Ssa::from_str(src).unwrap(); + let main = ssa.main_mut(); + + let instructions = main.dfg[main.entry_block()].instructions(); + assert_eq!(instructions.len(), 0); // The final return is not counted + + // `v6 = mul v0, v1` in b3 should now be `v3 = mul v0, v1` in b0 + let expected = " + brillig(inline) fn main f0 { + b0(v0: u32, v1: u32): + v3 = mul v0, v1 + jmp b1(u32 0) + b1(v2: u32): + v6 = lt v2, u32 4 + jmpif v6 then: b3, else: b2 + b3(): + constrain v3 == u32 6 + v9 = add v2, u32 1 + jmp b1(v9) + b2(): + return + } + "; + + let ssa = ssa.loop_invariant_code_motion(); + assert_normalized_ssa_equals(ssa, expected); + } + + #[test] + fn nested_loop_invariant_code_motion() { + // Check that a loop invariant in the inner loop of a nested loop + // is hoisted to the parent loop's pre-header block. + let src = " + brillig(inline) fn main f0 { + b0(v0: u32, v1: u32): + jmp b1(u32 0) + b1(v2: u32): + v6 = lt v2, u32 4 + jmpif v6 then: b3, else: b2 + b3(): + jmp b4(u32 0) + b4(v3: u32): + v7 = lt v3, u32 4 + jmpif v7 then: b6, else: b5 + b6(): + v10 = mul v0, v1 + constrain v10 == u32 6 + v12 = add v3, u32 1 + jmp b4(v12) + b5(): + v9 = add v2, u32 1 + jmp b1(v9) + b2(): + return + } + "; + + let mut ssa = Ssa::from_str(src).unwrap(); + let main = ssa.main_mut(); + + let instructions = main.dfg[main.entry_block()].instructions(); + assert_eq!(instructions.len(), 0); // The final return is not counted + + // `v10 = mul v0, v1` in b6 should now be `v4 = mul v0, v1` in b0 + let expected = " + brillig(inline) fn main f0 { + b0(v0: u32, v1: u32): + v4 = mul v0, v1 + jmp b1(u32 0) + b1(v2: u32): + v7 = lt v2, u32 4 + jmpif v7 then: b3, else: b2 + b3(): + jmp b4(u32 0) + b4(v3: u32): + v8 = lt v3, u32 4 + jmpif v8 then: b6, else: b5 + b6(): + constrain v4 == u32 6 + v12 = add v3, u32 1 + jmp b4(v12) + b5(): + v10 = add v2, u32 1 + jmp b1(v10) + b2(): + return + } + "; + + let ssa = ssa.loop_invariant_code_motion(); + assert_normalized_ssa_equals(ssa, expected); + } + + #[test] + fn hoist_invariant_with_invariant_as_argument() { + // Check that an instruction which has arguments defined in the loop + // but which are already marked loop invariants is still hoisted to the preheader. + // + // For example, in b3 we have the following instructions: + // ```text + // v6 = mul v0, v1 + // v7 = mul v6, v0 + // ``` + // `v6` should be marked a loop invariants as `v0` and `v1` are both declared outside of the loop. + // As we will be hoisting `v6 = mul v0, v1` to the loop preheader we know that we can also + // hoist `v7 = mul v6, v0`. + let src = " + brillig(inline) fn main f0 { + b0(v0: u32, v1: u32): + jmp b1(u32 0) + b1(v2: u32): + v5 = lt v2, u32 4 + jmpif v5 then: b3, else: b2 + b3(): + v6 = mul v0, v1 + v7 = mul v6, v0 + v8 = eq v7, u32 12 + constrain v7 == u32 12 + v9 = add v2, u32 1 + jmp b1(v9) + b2(): + return + } + "; + + let mut ssa = Ssa::from_str(src).unwrap(); + let main = ssa.main_mut(); + + let instructions = main.dfg[main.entry_block()].instructions(); + assert_eq!(instructions.len(), 0); // The final return is not counted + + let expected = " + brillig(inline) fn main f0 { + b0(v0: u32, v1: u32): + v3 = mul v0, v1 + v4 = mul v3, v0 + v6 = eq v4, u32 12 + jmp b1(u32 0) + b1(v2: u32): + v9 = lt v2, u32 4 + jmpif v9 then: b3, else: b2 + b3(): + constrain v4 == u32 12 + v11 = add v2, u32 1 + jmp b1(v11) + b2(): + return + } + "; + + let ssa = ssa.loop_invariant_code_motion(); + assert_normalized_ssa_equals(ssa, expected); + } + + #[test] + fn do_not_hoist_instructions_with_side_effects() { + // In `v12 = load v5` in `b3`, `v5` is defined outside the loop. + // However, as the instruction has side effects, we want to make sure + // we do not hoist the instruction to the loop preheader. + let src = " + brillig(inline) fn main f0 { + b0(v0: u32, v1: u32): + v4 = make_array [u32 0, u32 0, u32 0, u32 0, u32 0] : [u32; 5] + inc_rc v4 + v5 = allocate -> &mut [u32; 5] + store v4 at v5 + jmp b1(u32 0) + b1(v2: u32): + v7 = lt v2, u32 4 + jmpif v7 then: b3, else: b2 + b3(): + v12 = load v5 -> [u32; 5] + v13 = array_set v12, index v0, value v1 + store v13 at v5 + v15 = add v2, u32 1 + jmp b1(v15) + b2(): + v8 = load v5 -> [u32; 5] + v10 = array_get v8, index u32 2 -> u32 + constrain v10 == u32 3 + return + } + "; + + let mut ssa = Ssa::from_str(src).unwrap(); + let main = ssa.main_mut(); + + let instructions = main.dfg[main.entry_block()].instructions(); + assert_eq!(instructions.len(), 4); // The final return is not counted + + let ssa = ssa.loop_invariant_code_motion(); + // The code should be unchanged + assert_normalized_ssa_equals(ssa, src); + } +} diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/mod.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/mod.rs index 10e86c6601a..06481a12f60 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/mod.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/mod.rs @@ -12,6 +12,7 @@ mod defunctionalize; mod die; pub(crate) mod flatten_cfg; mod inlining; +mod loop_invariant; mod mem2reg; mod normalize_value_ids; mod rc; diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/simplify_cfg.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/simplify_cfg.rs index 46941775c5e..c282e2df451 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/simplify_cfg.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/simplify_cfg.rs @@ -18,7 +18,8 @@ use crate::ssa::{ basic_block::BasicBlockId, cfg::ControlFlowGraph, function::{Function, RuntimeType}, - instruction::TerminatorInstruction, + instruction::{Instruction, TerminatorInstruction}, + value::Value, }, ssa_gen::Ssa, }; @@ -31,6 +32,7 @@ impl Ssa { /// 4. Removing any blocks which have no instructions other than a single terminating jmp. /// 5. Replacing any jmpifs with constant conditions with jmps. If this causes the block to have /// only 1 successor then (2) also will be applied. + /// 6. Replacing any jmpifs with a negated condition with a jmpif with a un-negated condition and reversed branches. /// /// Currently, 1 is unimplemented. #[tracing::instrument(level = "trace", skip(self))] @@ -55,6 +57,8 @@ impl Function { stack.extend(self.dfg[block].successors().filter(|block| !visited.contains(block))); } + check_for_negated_jmpif_condition(self, block, &mut cfg); + // This call is before try_inline_into_predecessor so that if it succeeds in changing a // jmpif into a jmp, the block may then be inlined entirely into its predecessor in try_inline_into_predecessor. check_for_constant_jmpif(self, block, &mut cfg); @@ -184,6 +188,55 @@ fn check_for_double_jmp(function: &mut Function, block: BasicBlockId, cfg: &mut cfg.recompute_block(function, block); } +/// Optimize a jmpif on a negated condition by swapping the branches. +fn check_for_negated_jmpif_condition( + function: &mut Function, + block: BasicBlockId, + cfg: &mut ControlFlowGraph, +) { + if matches!(function.runtime(), RuntimeType::Acir(_)) { + // Swapping the `then` and `else` branches of a `JmpIf` within an ACIR function + // can result in the situation where the branches merge together again in the `then` block, e.g. + // + // acir(inline) fn main f0 { + // b0(v0: u1): + // jmpif v0 then: b2, else: b1 + // b2(): + // return + // b1(): + // jmp b2() + // } + // + // This breaks the `flatten_cfg` pass as it assumes that merges only happen in + // the `else` block or a 3rd block. + // + // See: https://github.com/noir-lang/noir/pull/5891#issuecomment-2500219428 + return; + } + + if let Some(TerminatorInstruction::JmpIf { + condition, + then_destination, + else_destination, + call_stack, + }) = function.dfg[block].terminator() + { + if let Value::Instruction { instruction, .. } = function.dfg[*condition] { + if let Instruction::Not(negated_condition) = function.dfg[instruction] { + let call_stack = call_stack.clone(); + let jmpif = TerminatorInstruction::JmpIf { + condition: negated_condition, + then_destination: *else_destination, + else_destination: *then_destination, + call_stack, + }; + function.dfg[block].set_terminator(jmpif); + cfg.recompute_block(function, block); + } + } + } +} + /// If the given block has block parameters, replace them with the jump arguments from the predecessor. /// /// Currently, if this function is needed, `try_inline_into_predecessor` will also always apply, @@ -246,6 +299,8 @@ mod test { map::Id, types::Type, }, + opt::assert_normalized_ssa_equals, + Ssa, }; use acvm::acir::AcirField; @@ -359,4 +414,59 @@ mod test { other => panic!("Unexpected terminator {other:?}"), } } + + #[test] + fn swap_negated_jmpif_branches_in_brillig() { + let src = " + brillig(inline) fn main f0 { + b0(v0: u1): + v1 = allocate -> &mut Field + store Field 0 at v1 + v3 = not v0 + jmpif v3 then: b1, else: b2 + b1(): + store Field 2 at v1 + jmp b2() + b2(): + v5 = load v1 -> Field + v6 = eq v5, Field 2 + constrain v5 == Field 2 + return + }"; + let ssa = Ssa::from_str(src).unwrap(); + + let expected = " + brillig(inline) fn main f0 { + b0(v0: u1): + v1 = allocate -> &mut Field + store Field 0 at v1 + v3 = not v0 + jmpif v0 then: b2, else: b1 + b2(): + v5 = load v1 -> Field + v6 = eq v5, Field 2 + constrain v5 == Field 2 + return + b1(): + store Field 2 at v1 + jmp b2() + }"; + assert_normalized_ssa_equals(ssa.simplify_cfg(), expected); + } + + #[test] + fn does_not_swap_negated_jmpif_branches_in_acir() { + let src = " + acir(inline) fn main f0 { + b0(v0: u1): + v1 = not v0 + jmpif v1 then: b1, else: b2 + b1(): + jmp b2() + b2(): + return + }"; + let ssa = Ssa::from_str(src).unwrap(); + assert_normalized_ssa_equals(ssa.simplify_cfg(), src); + } } diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs index 89f1b2b2d7d..44e25f9d4a1 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs @@ -84,7 +84,7 @@ impl Function { } } -struct Loop { +pub(super) struct Loop { /// The header block of a loop is the block which dominates all the /// other blocks in the loop. header: BasicBlockId, @@ -94,17 +94,17 @@ struct Loop { back_edge_start: BasicBlockId, /// All the blocks contained within the loop, including `header` and `back_edge_start`. - blocks: HashSet, + pub(super) blocks: HashSet, } -struct Loops { +pub(super) struct Loops { /// The loops that failed to be unrolled so that we do not try to unroll them again. /// Each loop is identified by its header block id. failed_to_unroll: HashSet, - yet_to_unroll: Vec, + pub(super) yet_to_unroll: Vec, modified_blocks: HashSet, - cfg: ControlFlowGraph, + pub(super) cfg: ControlFlowGraph, } impl Loops { @@ -136,7 +136,7 @@ impl Loops { /// loop_end loop_body /// ``` /// `loop_entry` has two predecessors: `main` and `loop_body`, and it dominates `loop_body`. - fn find_all(function: &Function) -> Self { + pub(super) fn find_all(function: &Function) -> Self { let cfg = ControlFlowGraph::with_function(function); let post_order = PostOrder::with_function(function); let mut dom_tree = DominatorTree::with_cfg_and_post_order(&cfg, &post_order); @@ -393,7 +393,7 @@ impl Loop { /// The loop pre-header is the block that comes before the loop begins. Generally a header block /// is expected to have 2 predecessors: the pre-header and the final block of the loop which jumps /// back to the beginning. Other predecessors can come from `break` or `continue`. - fn get_pre_header( + pub(super) fn get_pre_header( &self, function: &Function, cfg: &ControlFlowGraph, diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/parser/ast.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/parser/ast.rs index a34b7fd70d3..6c7608a2f16 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/parser/ast.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/parser/ast.rs @@ -89,6 +89,7 @@ pub(crate) enum ParsedInstruction { Constrain { lhs: ParsedValue, rhs: ParsedValue, + assert_message: Option, }, DecrementRc { value: ParsedValue, @@ -129,6 +130,12 @@ pub(crate) enum ParsedInstruction { }, } +#[derive(Debug)] +pub(crate) enum AssertMessage { + Static(String), + Dynamic(Vec), +} + #[derive(Debug)] pub(crate) enum ParsedTerminator { Jmp { destination: Identifier, arguments: Vec }, diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/parser/into_ssa.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/parser/into_ssa.rs index 552ac0781c7..e78cbbd75a1 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/parser/into_ssa.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/parser/into_ssa.rs @@ -1,13 +1,18 @@ use std::collections::HashMap; +use acvm::acir::circuit::ErrorSelector; + use crate::ssa::{ function_builder::FunctionBuilder, - ir::{basic_block::BasicBlockId, function::FunctionId, value::ValueId}, + ir::{ + basic_block::BasicBlockId, function::FunctionId, instruction::ConstrainError, + value::ValueId, + }, }; use super::{ - Identifier, ParsedBlock, ParsedFunction, ParsedInstruction, ParsedSsa, ParsedTerminator, - ParsedValue, RuntimeType, Ssa, SsaError, + ast::AssertMessage, Identifier, ParsedBlock, ParsedFunction, ParsedInstruction, ParsedSsa, + ParsedTerminator, ParsedValue, RuntimeType, Ssa, SsaError, }; impl ParsedSsa { @@ -31,6 +36,8 @@ struct Translator { /// passes already which replaced some of the original IDs. The translator /// will recreate the SSA step by step, which can result in a new ID layout. variables: HashMap>, + + error_selector_counter: u64, } impl Translator { @@ -64,8 +71,13 @@ impl Translator { functions.insert(function.internal_name.clone(), function_id); } - let mut translator = - Self { builder, functions, variables: HashMap::new(), blocks: HashMap::new() }; + let mut translator = Self { + builder, + functions, + variables: HashMap::new(), + blocks: HashMap::new(), + error_selector_counter: 0, + }; translator.translate_function_body(main_function)?; Ok(translator) @@ -198,10 +210,25 @@ impl Translator { let value_id = self.builder.insert_cast(lhs, typ); self.define_variable(target, value_id)?; } - ParsedInstruction::Constrain { lhs, rhs } => { + ParsedInstruction::Constrain { lhs, rhs, assert_message } => { let lhs = self.translate_value(lhs)?; let rhs = self.translate_value(rhs)?; - self.builder.insert_constrain(lhs, rhs, None); + let assert_message = match assert_message { + Some(AssertMessage::Static(string)) => { + Some(ConstrainError::StaticString(string)) + } + Some(AssertMessage::Dynamic(values)) => { + let error_selector = ErrorSelector::new(self.error_selector_counter); + self.error_selector_counter += 1; + + let is_string_type = false; + let values = self.translate_values(values)?; + + Some(ConstrainError::Dynamic(error_selector, is_string_type, values)) + } + None => None, + }; + self.builder.insert_constrain(lhs, rhs, assert_message); } ParsedInstruction::DecrementRc { value } => { let value = self.translate_value(value)?; diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/parser/lexer.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/parser/lexer.rs index 4c90475be74..d89bc1e9e28 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/parser/lexer.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/parser/lexer.rs @@ -61,6 +61,7 @@ impl<'a> Lexer<'a> { Some('&') => self.single_char_token(Token::Ampersand), Some('-') if self.peek_char() == Some('>') => self.double_char_token(Token::Arrow), Some('-') => self.single_char_token(Token::Dash), + Some('"') => self.eat_string_literal(), Some(ch) if ch.is_ascii_alphanumeric() || ch == '_' => self.eat_alpha_numeric(ch), Some(char) => Err(LexerError::UnexpectedCharacter { char, @@ -177,6 +178,41 @@ impl<'a> Lexer<'a> { Ok(integer_token.into_span(start, end)) } + fn eat_string_literal(&mut self) -> SpannedTokenResult { + let start = self.position; + let mut string = String::new(); + + while let Some(next) = self.next_char() { + let char = match next { + '"' => break, + '\\' => match self.next_char() { + Some('r') => '\r', + Some('n') => '\n', + Some('t') => '\t', + Some('0') => '\0', + Some('"') => '"', + Some('\\') => '\\', + Some(escaped) => { + let span = Span::inclusive(start, self.position); + return Err(LexerError::InvalidEscape { escaped, span }); + } + None => { + let span = Span::inclusive(start, self.position); + return Err(LexerError::UnterminatedStringLiteral { span }); + } + }, + other => other, + }; + + string.push(char); + } + + let str_literal_token = Token::Str(string); + + let end = self.position; + Ok(str_literal_token.into_span(start, end)) + } + fn eat_while bool>( &mut self, initial_char: Option, @@ -247,6 +283,12 @@ pub(crate) enum LexerError { InvalidIntegerLiteral { span: Span, found: String }, #[error("Integer literal too large")] IntegerLiteralTooLarge { span: Span, limit: String }, + #[error("Unterminated string literal")] + UnterminatedStringLiteral { span: Span }, + #[error( + "'\\{escaped}' is not a valid escape sequence. Use '\\' for a literal backslash character." + )] + InvalidEscape { escaped: char, span: Span }, } impl LexerError { @@ -254,7 +296,9 @@ impl LexerError { match self { LexerError::UnexpectedCharacter { span, .. } | LexerError::InvalidIntegerLiteral { span, .. } - | LexerError::IntegerLiteralTooLarge { span, .. } => *span, + | LexerError::IntegerLiteralTooLarge { span, .. } + | LexerError::UnterminatedStringLiteral { span } + | LexerError::InvalidEscape { span, .. } => *span, } } } diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/parser/mod.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/parser/mod.rs index 2db2c636a8f..3d8bd37dead 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/parser/mod.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/parser/mod.rs @@ -10,8 +10,8 @@ use super::{ use acvm::{AcirField, FieldElement}; use ast::{ - Identifier, ParsedBlock, ParsedFunction, ParsedInstruction, ParsedParameter, ParsedSsa, - ParsedValue, + AssertMessage, Identifier, ParsedBlock, ParsedFunction, ParsedInstruction, ParsedParameter, + ParsedSsa, ParsedValue, }; use lexer::{Lexer, LexerError}; use noirc_errors::Span; @@ -28,6 +28,11 @@ mod tests; mod token; impl Ssa { + /// Creates an Ssa object from the given string. + /// + /// Note that the resulting Ssa might not be exactly the same as the given string. + /// This is because, internally, the Ssa is built using a `FunctionBuilder`, so + /// some instructions might be simplified while they are inserted. pub(crate) fn from_str(src: &str) -> Result { let mut parser = Parser::new(src).map_err(|err| SsaErrorWithSource::parse_error(err, src))?; @@ -308,7 +313,20 @@ impl<'a> Parser<'a> { let lhs = self.parse_value_or_error()?; self.eat_or_error(Token::Equal)?; let rhs = self.parse_value_or_error()?; - Ok(Some(ParsedInstruction::Constrain { lhs, rhs })) + + let assert_message = if self.eat(Token::Comma)? { + if let Some(str) = self.eat_str()? { + Some(AssertMessage::Static(str)) + } else if self.eat_keyword(Keyword::Data)? { + Some(AssertMessage::Dynamic(self.parse_comma_separated_values()?)) + } else { + return self.expected_string_or_data(); + } + } else { + None + }; + + Ok(Some(ParsedInstruction::Constrain { lhs, rhs, assert_message })) } fn parse_decrement_rc(&mut self) -> ParseResult> { @@ -649,6 +667,10 @@ impl<'a> Parser<'a> { return Ok(Type::Reference(Arc::new(typ))); } + if self.eat_keyword(Keyword::Function)? { + return Ok(Type::Function); + } + self.expected_type() } @@ -762,6 +784,18 @@ impl<'a> Parser<'a> { } } + fn eat_str(&mut self) -> ParseResult> { + if matches!(self.token.token(), Token::Str(..)) { + let token = self.bump()?; + match token.into_token() { + Token::Str(string) => Ok(Some(string)), + _ => unreachable!(), + } + } else { + Ok(None) + } + } + fn eat(&mut self, token: Token) -> ParseResult { if self.token.token() == &token { self.bump()?; @@ -807,6 +841,13 @@ impl<'a> Parser<'a> { }) } + fn expected_string_or_data(&mut self) -> ParseResult { + Err(ParserError::ExpectedStringOrData { + found: self.token.token().clone(), + span: self.token.to_span(), + }) + } + fn expected_identifier(&mut self) -> ParseResult { Err(ParserError::ExpectedIdentifier { found: self.token.token().clone(), @@ -868,6 +909,8 @@ pub(crate) enum ParserError { ExpectedType { found: Token, span: Span }, #[error("Expected an instruction or terminator, found '{found}'")] ExpectedInstructionOrTerminator { found: Token, span: Span }, + #[error("Expected a string literal or 'data', found '{found}'")] + ExpectedStringOrData { found: Token, span: Span }, #[error("Expected a value, found '{found}'")] ExpectedValue { found: Token, span: Span }, #[error("Multiple return values only allowed for call")] @@ -884,6 +927,7 @@ impl ParserError { | ParserError::ExpectedInt { span, .. } | ParserError::ExpectedType { span, .. } | ParserError::ExpectedInstructionOrTerminator { span, .. } + | ParserError::ExpectedStringOrData { span, .. } | ParserError::ExpectedValue { span, .. } => *span, ParserError::MultipleReturnValuesOnlyAllowedForCall { second_target, .. } => { second_target.span diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/parser/tests.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/parser/tests.rs index 60d398bf9d5..593b66d0c98 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/parser/tests.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/parser/tests.rs @@ -214,6 +214,31 @@ fn test_constrain() { assert_ssa_roundtrip(src); } +#[test] +fn test_constrain_with_static_message() { + let src = r#" + acir(inline) fn main f0 { + b0(v0: Field): + constrain v0 == Field 1, "Oh no!" + return + } + "#; + assert_ssa_roundtrip(src); +} + +#[test] +fn test_constrain_with_dynamic_message() { + let src = " + acir(inline) fn main f0 { + b0(v0: Field, v1: Field): + v7 = make_array [u8 123, u8 120, u8 125, u8 32, u8 123, u8 121, u8 125] : [u8; 7] + constrain v0 == Field 1, data v7, u32 2, v0, v1 + return + } + "; + assert_ssa_roundtrip(src); +} + #[test] fn test_enable_side_effects() { let src = " @@ -441,3 +466,15 @@ fn test_negative() { "; assert_ssa_roundtrip(src); } + +#[test] +fn test_function_type() { + let src = " + acir(inline) fn main f0 { + b0(): + v0 = allocate -> &mut function + return + } + "; + assert_ssa_roundtrip(src); +} diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/parser/token.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/parser/token.rs index f663879e899..d8dd4ec011e 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/parser/token.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/parser/token.rs @@ -29,6 +29,7 @@ impl SpannedToken { pub(crate) enum Token { Ident(String), Int(FieldElement), + Str(String), Keyword(Keyword), IntType(IntType), /// = @@ -77,6 +78,7 @@ impl Display for Token { match self { Token::Ident(ident) => write!(f, "{}", ident), Token::Int(int) => write!(f, "{}", int), + Token::Str(string) => write!(f, "{string:?}"), Token::Keyword(keyword) => write!(f, "{}", keyword), Token::IntType(int_type) => write!(f, "{}", int_type), Token::Assign => write!(f, "="), @@ -120,6 +122,7 @@ pub(crate) enum Keyword { Call, Cast, Constrain, + Data, DecRc, Div, Inline, @@ -130,6 +133,7 @@ pub(crate) enum Keyword { Field, Fold, Fn, + Function, IncRc, Index, Jmp, @@ -175,6 +179,7 @@ impl Keyword { "call" => Keyword::Call, "cast" => Keyword::Cast, "constrain" => Keyword::Constrain, + "data" => Keyword::Data, "dec_rc" => Keyword::DecRc, "div" => Keyword::Div, "else" => Keyword::Else, @@ -185,6 +190,7 @@ impl Keyword { "Field" => Keyword::Field, "fold" => Keyword::Fold, "fn" => Keyword::Fn, + "function" => Keyword::Function, "inc_rc" => Keyword::IncRc, "index" => Keyword::Index, "jmp" => Keyword::Jmp, @@ -234,6 +240,7 @@ impl Display for Keyword { Keyword::Call => write!(f, "call"), Keyword::Cast => write!(f, "cast"), Keyword::Constrain => write!(f, "constrain"), + Keyword::Data => write!(f, "data"), Keyword::DecRc => write!(f, "dec_rc"), Keyword::Div => write!(f, "div"), Keyword::Else => write!(f, "else"), @@ -242,6 +249,7 @@ impl Display for Keyword { Keyword::Field => write!(f, "Field"), Keyword::Fold => write!(f, "fold"), Keyword::Fn => write!(f, "fn"), + Keyword::Function => write!(f, "function"), Keyword::IncRc => write!(f, "inc_rc"), Keyword::Index => write!(f, "index"), Keyword::Inline => write!(f, "inline"), diff --git a/noir/noir-repo/compiler/noirc_frontend/src/parser/errors.rs b/noir/noir-repo/compiler/noirc_frontend/src/parser/errors.rs index bcb4ce1c616..899928528e6 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/parser/errors.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/parser/errors.rs @@ -19,6 +19,8 @@ pub enum ParserErrorReason { UnexpectedComma, #[error("Expected a `{token}` separating these two {items}")] ExpectedTokenSeparatingTwoItems { token: Token, items: &'static str }, + #[error("Expected `mut` after `&`, found `{found}`")] + ExpectedMutAfterAmpersand { found: Token }, #[error("Invalid left-hand side of assignment")] InvalidLeftHandSideOfAssignment, #[error("Expected trait, found {found}")] @@ -265,6 +267,11 @@ impl<'a> From<&'a ParserError> for Diagnostic { error.span, ), ParserErrorReason::Lexer(error) => error.into(), + ParserErrorReason::ExpectedMutAfterAmpersand { found } => Diagnostic::simple_error( + format!("Expected `mut` after `&`, found `{found}`"), + "Noir doesn't have immutable references, only mutable references".to_string(), + error.span, + ), other => Diagnostic::simple_error(format!("{other}"), String::new(), error.span), }, None => { diff --git a/noir/noir-repo/compiler/noirc_frontend/src/parser/parser.rs b/noir/noir-repo/compiler/noirc_frontend/src/parser/parser.rs index f369839ddd4..c2f7b781873 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/parser/parser.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/parser/parser.rs @@ -498,6 +498,13 @@ impl<'a> Parser<'a> { self.push_error(ParserErrorReason::ExpectedTokenSeparatingTwoItems { token, items }, span); } + fn expected_mut_after_ampersand(&mut self) { + self.push_error( + ParserErrorReason::ExpectedMutAfterAmpersand { found: self.token.token().clone() }, + self.current_token_span, + ); + } + fn modifiers_not_followed_by_an_item(&mut self, modifiers: Modifiers) { self.visibility_not_followed_by_an_item(modifiers); self.unconstrained_not_followed_by_an_item(modifiers); diff --git a/noir/noir-repo/compiler/noirc_frontend/src/parser/parser/types.rs b/noir/noir-repo/compiler/noirc_frontend/src/parser/parser/types.rs index be3d5287cab..0de94a89be5 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/parser/parser/types.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/parser/parser/types.rs @@ -341,7 +341,10 @@ impl<'a> Parser<'a> { fn parses_mutable_reference_type(&mut self) -> Option { if self.eat(Token::Ampersand) { - self.eat_keyword_or_error(Keyword::Mut); + if !self.eat_keyword(Keyword::Mut) { + self.expected_mut_after_ampersand(); + } + return Some(UnresolvedTypeData::MutableReference(Box::new( self.parse_type_or_error(), ))); diff --git a/noir/noir-repo/tooling/lsp/src/requests/code_action/import_or_qualify.rs b/noir/noir-repo/tooling/lsp/src/requests/code_action/import_or_qualify.rs index ffc83b05a5b..609a81bdfe7 100644 --- a/noir/noir-repo/tooling/lsp/src/requests/code_action/import_or_qualify.rs +++ b/noir/noir-repo/tooling/lsp/src/requests/code_action/import_or_qualify.rs @@ -183,6 +183,31 @@ mod foo { } } +fn foo(x: SomeTypeInBar) {}"#; + + assert_code_action(title, src, expected).await; + } + + #[test] + async fn test_import_code_action_for_struct_at_beginning_of_name() { + let title = "Import foo::bar::SomeTypeInBar"; + + let src = r#"mod foo { + pub mod bar { + pub struct SomeTypeInBar {} + } +} + +fn foo(x: >|