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.

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

feat(perf): mem2reg function state for value loads to optimize across blocks #5757

Merged
merged 28 commits into from
Aug 19, 2024
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
c7f70de
brillig opcode report workflow and small changes to info cmd for serde
vezenovm Aug 16, 2024
8acce99
update noir-gates-diff commit and chmod gates_report_brillig.sh
vezenovm Aug 16, 2024
d962871
increase circuit to test gates reports
vezenovm Aug 16, 2024
e54b80a
fix compile_success_empty
vezenovm Aug 16, 2024
e24932e
fix compile_success_empty
vezenovm Aug 16, 2024
e41b1d3
Merge branch 'master' into mv/brillig-opcode-report
vezenovm Aug 16, 2024
6b70236
Merge branch 'mv/brillig-opcode-report' into mv/test-brillig-opcode-r…
vezenovm Aug 16, 2024
60bbbdc
update header for brillig opcode report
vezenovm Aug 16, 2024
d65b066
use brillig_bytecode_diff for job id
vezenovm Aug 16, 2024
63b3d02
update commit
vezenovm Aug 16, 2024
b2be50e
Merge branch 'mv/brillig-opcode-report' into mv/test-brillig-opcode-r…
TomAFrench Aug 19, 2024
0ebc06e
add headers to the sticky comments
vezenovm Aug 19, 2024
85f4017
remove keccak256 test additions and add size regression from issue #4535
vezenovm Aug 19, 2024
4753d65
track the last loads across a function to cleanup blocks w/ unused st…
vezenovm Aug 19, 2024
f6342e4
update commit and pass brillig_report flag to github action
vezenovm Aug 19, 2024
7083c1e
Merge branch 'mv/test-brillig-opcode-report' into mv/mem2reg-small-fu…
vezenovm Aug 19, 2024
3a89d8e
udpate comment
vezenovm Aug 19, 2024
d978889
only insert last_load when load value is unknown
vezenovm Aug 19, 2024
5a024c4
fix mem2reg test after opt
vezenovm Aug 19, 2024
87d6f0b
update multiple_blocks test
vezenovm Aug 19, 2024
6472f23
add regression for issue 5761
vezenovm Aug 19, 2024
7d9612b
revert test added to wrong branch
vezenovm Aug 19, 2024
af35285
conflicts w/ master
vezenovm Aug 19, 2024
487879b
check we do not have a reference param
vezenovm Aug 19, 2024
cdfd93f
remvoe debug println
vezenovm Aug 19, 2024
fb49590
clippy
vezenovm Aug 19, 2024
8b56fb5
Update compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs
vezenovm Aug 19, 2024
c1ef620
missing semicolon
vezenovm Aug 19, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/gates_report.yml
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ jobs:

- name: Compare gates reports
id: gates_diff
uses: vezenovm/noir-gates-diff@acf12797860f237117e15c0d6e08d64253af52b6
uses: noir-lang/noir-gates-diff@1931aaaa848a1a009363d6115293f7b7fc72bb87
with:
report: gates_report.json
summaryQuantile: 0.9 # only display the 10% most significant circuit size diffs in the summary (defaults to 20%)
Expand Down
92 changes: 92 additions & 0 deletions .github/workflows/gates_report_brillig.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
name: Report Brillig bytecode size diff

on:
push:
branches:
- master
pull_request:

jobs:
build-nargo:
runs-on: ubuntu-latest
strategy:
matrix:
target: [x86_64-unknown-linux-gnu]

steps:
- name: Checkout Noir repo
uses: actions/checkout@v4

- name: Setup toolchain
uses: dtolnay/rust-toolchain@1.74.1

- uses: Swatinem/rust-cache@v2
with:
key: ${{ matrix.target }}
cache-on-failure: true
save-if: ${{ github.event_name != 'merge_group' }}

- name: Build Nargo
run: cargo build --package nargo_cli --release

- name: Package artifacts
run: |
mkdir dist
cp ./target/release/nargo ./dist/nargo
7z a -ttar -so -an ./dist/* | 7z a -si ./nargo-x86_64-unknown-linux-gnu.tar.gz

- name: Upload artifact
uses: actions/upload-artifact@v4
with:
name: nargo
path: ./dist/*
retention-days: 3

compare_brillig_bytecode_size_reports:
needs: [build-nargo]
runs-on: ubuntu-latest
permissions:
pull-requests: write

steps:
- uses: actions/checkout@v4

- name: Download nargo binary
uses: actions/download-artifact@v4
with:
name: nargo
path: ./nargo

- name: Set nargo on PATH
run: |
nargo_binary="${{ github.workspace }}/nargo/nargo"
chmod +x $nargo_binary
echo "$(dirname $nargo_binary)" >> $GITHUB_PATH
export PATH="$PATH:$(dirname $nargo_binary)"
nargo -V

- name: Generate Brillig bytecode size report
working-directory: ./test_programs
run: |
chmod +x gates_report_brillig.sh
./gates_report_brillig.sh
mv gates_report_brillig.json ../gates_report_brillig.json

- name: Compare Brillig bytecode size reports
id: brillig_bytecode_diff
uses: noir-lang/noir-gates-diff@79010d656ae50e83a28e5139ed3850b8cc9873d1
with:
report: gates_report_brillig.json
header: |
# Changes to Brillig bytecode sizes
brillig_report: true
summaryQuantile: 0.9 # only display the 10% most significant bytecode size diffs in the summary (defaults to 20%)

- name: Add bytecode size diff to sticky comment
if: github.event_name == 'pull_request' || github.event_name == 'pull_request_target'
uses: marocchino/sticky-pull-request-comment@v2
with:
header: brillig
# delete the comment in case changes no longer impact brillig bytecode sizes
delete: ${{ !steps.brillig_bytecode_diff.outputs.markdown }}
message: ${{ steps.brillig_bytecode_diff.outputs.markdown }}
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ tooling/noir_js/lib
!compiler/wasm/noir-script/target

gates_report.json
gates_report_brillig.json

# Github Actions scratch space
# This gives a location to download artifacts into the repository in CI without making git dirty.
Expand Down
17 changes: 17 additions & 0 deletions compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@
//!
//! Repeating this algorithm for each block in the function in program order should result in
//! optimizing out most known loads. However, identifying all aliases correctly has been proven
//! undecidable in general (Landi, 1992). So this pass will not always optimize out all loads

Check warning on line 59 in compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (Landi)
//! that could theoretically be optimized out. This pass can be performed at any time in the
//! SSA optimization pipeline, although it will be more successful the simpler the program's CFG is.
//! This pass is currently performed several times to enable other passes - most notably being
Expand All @@ -66,6 +66,8 @@

use std::collections::{BTreeMap, BTreeSet};

use fxhash::FxHashMap as HashMap;

use crate::ssa::{
ir::{
basic_block::BasicBlockId,
Expand Down Expand Up @@ -109,8 +111,12 @@
/// Load and Store instructions that should be removed at the end of the pass.
///
/// We avoid removing individual instructions as we go since removing elements
/// from the middle of Vecs many times will be slower than a single call to `retain`.

Check warning on line 114 in compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (Vecs)
instructions_to_remove: BTreeSet<InstructionId>,

/// Track a value's last load across all blocks.
/// If a value is not used in anymore loads we can remove the last store to that value.
last_loads: HashMap<ValueId, InstructionId>,
}

impl<'f> PerFunctionContext<'f> {
Expand All @@ -124,6 +130,7 @@
inserter: FunctionInserter::new(function),
blocks: BTreeMap::new(),
instructions_to_remove: BTreeSet::new(),
last_loads: HashMap::default(),
}
}

Expand All @@ -140,6 +147,14 @@
let references = self.find_starting_references(block);
self.analyze_block(block, references);
}

for (_, block) in self.blocks.iter() {
for (value, store_instruction) in block.last_stores.iter() {
if self.last_loads.get(value).is_none() {
self.instructions_to_remove.insert(*store_instruction);
}
}
}
jfecher marked this conversation as resolved.
Show resolved Hide resolved
}

/// The value of each reference at the start of the given block is the unification
Expand Down Expand Up @@ -239,6 +254,8 @@
self.instructions_to_remove.insert(instruction);
} else {
references.mark_value_used(address, self.inserter.function);

self.last_loads.insert(address, instruction);
}
}
Instruction::Store { address, value } => {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
[package]
name = "brillig_loop_size_regression"
type = "bin"
authors = [""]
compiler_version = ">=0.33.0"

[dependencies]
Empty file.
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
struct EnumEmulation {
a: Option<Field>,
b: Option<Field>,
c: Option<Field>,
}

unconstrained fn main() -> pub Field {
let mut emulated_enum = EnumEmulation { a: Option::some(1), b: Option::none(), c: Option::none() };

for _ in 0..1 {
assert_eq(emulated_enum.a.unwrap(), 1);
}

emulated_enum.a = Option::some(2);
emulated_enum.a.unwrap()
}
33 changes: 33 additions & 0 deletions test_programs/gates_report_brillig.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
#!/usr/bin/env bash
set -e

# These tests are incompatible with gas reporting
excluded_dirs=("workspace" "workspace_default_member" "double_verify_nested_proof" "overlapping_dep_and_mod" "comptime_println")

current_dir=$(pwd)
base_path="$current_dir/execution_success"
test_dirs=$(ls $base_path)

# We generate a Noir workspace which contains all of the test cases
# This allows us to generate a gates report using `nargo info` for all of them at once.

echo "[workspace]" > Nargo.toml
echo "members = [" >> Nargo.toml

for dir in $test_dirs; do
if [[ " ${excluded_dirs[@]} " =~ " ${dir} " ]]; then
continue
fi

if [[ ${CI-false} = "true" ]] && [[ " ${ci_excluded_dirs[@]} " =~ " ${dir} " ]]; then
continue
fi

echo " \"execution_success/$dir\"," >> Nargo.toml
done

echo "]" >> Nargo.toml

nargo info --force-brillig --json > gates_report_brillig.json

rm Nargo.toml
2 changes: 1 addition & 1 deletion tooling/nargo_cli/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,7 @@ fn generate_compile_success_empty_tests(test_file: &mut File, test_data_dir: &Pa
let json: serde_json::Value = serde_json::from_slice(&output.stdout).unwrap_or_else(|e| {{
panic!("JSON was not well-formatted {:?}\n\n{:?}", e, std::str::from_utf8(&output.stdout))
}});
let num_opcodes = &json["programs"][0]["functions"][0]["acir_opcodes"];
let num_opcodes = &json["programs"][0]["functions"][0]["opcodes"];
assert_eq!(num_opcodes.as_u64().expect("number of opcodes should fit in a u64"), 0);
"#;

Expand Down
13 changes: 7 additions & 6 deletions tooling/nargo_cli/src/cli/info_cmd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,7 @@ struct ProgramInfo {
#[serde(skip)]
expression_width: ExpressionWidth,
functions: Vec<FunctionInfo>,
#[serde(skip)]
unconstrained_functions_opcodes: usize,
unconstrained_functions: Vec<FunctionInfo>,
}
Expand All @@ -186,7 +187,7 @@ impl From<ProgramInfo> for Vec<Row> {
Fm->format!("{}", program_info.package_name),
Fc->format!("{}", function.name),
format!("{:?}", program_info.expression_width),
Fc->format!("{}", function.acir_opcodes),
Fc->format!("{}", function.opcodes),
Fc->format!("{}", program_info.unconstrained_functions_opcodes),
]
});
Expand All @@ -196,7 +197,7 @@ impl From<ProgramInfo> for Vec<Row> {
Fc->format!("{}", function.name),
format!("N/A", ),
Fc->format!("N/A"),
Fc->format!("{}", function.acir_opcodes),
Fc->format!("{}", function.opcodes),
]
}));
main
Expand All @@ -215,7 +216,7 @@ struct ContractInfo {
#[derive(Debug, Serialize)]
struct FunctionInfo {
name: String,
acir_opcodes: usize,
opcodes: usize,
}

impl From<ContractInfo> for Vec<Row> {
Expand All @@ -225,7 +226,7 @@ impl From<ContractInfo> for Vec<Row> {
Fm->format!("{}", contract_info.name),
Fc->format!("{}", function.name),
format!("{:?}", contract_info.expression_width),
Fc->format!("{}", function.acir_opcodes),
Fc->format!("{}", function.opcodes),
]
})
}
Expand All @@ -243,7 +244,7 @@ fn count_opcodes_and_gates_in_program(
.enumerate()
.map(|(i, function)| FunctionInfo {
name: compiled_program.names[i].clone(),
acir_opcodes: function.opcodes.len(),
opcodes: function.opcodes.len(),
})
.collect();

Expand All @@ -264,7 +265,7 @@ fn count_opcodes_and_gates_in_program(
.clone()
.iter()
.zip(opcodes_len)
.map(|(name, len)| FunctionInfo { name: name.clone(), acir_opcodes: len })
.map(|(name, len)| FunctionInfo { name: name.clone(), opcodes: len })
.collect();

ProgramInfo {
Expand Down
Loading