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

fix: Change PoseidonHasher::write to be inline_always #6468

Merged
merged 2 commits into from
Nov 8, 2024

Conversation

aakoshh
Copy link
Contributor

@aakoshh aakoshh commented Nov 7, 2024

Description

Problem*

Resolves #6441

Summary*

Adds the #[inline_always] attribute to PoseidonHasher::write. This way at least we can pass the tests in #6429 for --inliner-aggressiveness 0, which is what was supposed to be the default value in #6378 We still have to exclude the -Inf option in the tests.

Looking behind the scenes, many of the functions in the test have inlining costs slightly less than zero, so with -Inf they don't get inlined, but with 0 they do, but that is not enough to make the test pass. An aggressiveness of 22 is enough; as it happens write has a cost of 21, and inlining this method does the trick.

I don't know if it makes sense to add this attribute, but wanted to open a PR to discuss.

The PR targets the test-matrix branch and changes the test generation to go through the whole possible range of aggressiveness for ACIR but allow us to override the minimum for Brillig.

Additional Context

The SSA generated contains huge arrays coming from the various poseidon::bn254::consts::x5_[2-16]_config functions, and Noir crashes trying to allocate more than MAX_STACK_FRAME_SIZE registers for these constants. With -Inf it dies after allocating ~2200 registers, with 0 it goes up to over ~15000, I assume across different stack frames.

With +Inf it looks like as if it only kept 11 out of the 15 arrays (just scrolling down the SSA and counting the huge swathes of number blocks 👀), and only creates ~800 registers.

For the record the SSA of just the write function looks like this:

acir(inline) fn write f12 {
  b0(v0: &mut u32, v1: &mut [Field], v2: Field):
    v3 = load v0
    v4 = load v1
    v5 = load v0
    v6 = load v1
    v8, v9 = call slice_push_back(v5, v6, v2)
    inc_rc v9
    store v8 at v0
    store v9 at v1
    return 
}

A working hypothesis is that PoseidonHasher::finish() has a big if-else for each of the 15 different acceptable lengths it can handle, each of which have a different set of constants. If we inline write then the compiler doesn't lose track of which slice is being mutated, and can eliminate the unneeded constants.

It's worth trying to run the program with --show-brillig (which prints the brillig opcodes as it creates them) with --inliner-aggressivness 22 and 0 (without adding the attribute). With aggressiveness 22 the entire brillig opcode is maybe 4K lines, while with 0 it dies after 15K lines. It shows all the constants being created, which seem to come from a chain of blocks jumping from one to the next. I thought it might be the fact that extra copies are needed if write is a separate function, due to shared references in the function input. write is used to hash the signatures and the input together here.

Documentation*

Check one:

  • No documentation needed.
  • Documentation included in this PR.
  • [For Experimental Features] Documentation to be submitted in a separate PR.

PR Checklist*

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

@aakoshh aakoshh changed the title Change PoseidonHasher::write to be inline_always fix: Change PoseidonHasher::write to be inline_always Nov 7, 2024
@aakoshh aakoshh force-pushed the 6441-fix-eddsa-inline-write branch from 153a7d3 to 7312b7f Compare November 7, 2024 12:45
@aakoshh aakoshh changed the base branch from master to 6390-inliner-test-matrix November 7, 2024 12:45
@aakoshh aakoshh merged commit 3991100 into 6390-inliner-test-matrix Nov 8, 2024
61 of 63 checks passed
@aakoshh aakoshh deleted the 6441-fix-eddsa-inline-write branch November 8, 2024 11:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant