Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

chore: resolve Instruction inputs fully before checking against cache #2472

Merged
merged 3 commits into from
Aug 29, 2023
Merged

chore: resolve Instruction inputs fully before checking against cache #2472

merged 3 commits into from
Aug 29, 2023

Conversation

TomAFrench
Copy link
Member

@TomAFrench TomAFrench commented Aug 29, 2023 •
edited
Loading

Description

Problem*

Resolves

Summary*

It struck me that we should be fully resolving any inputs to the instruction before we check against the cache. This allows us to catch the maximum amount of duplicate instructions.

What's concerning is that I'm now getting a failed constraint error in the sha2 tests. This seems otherwise a fairly low-risk change so I'm not sure if there's a deeper issue in #2450 which wasn't caught which means we should revert that change. @jfecher what do you think?

Documentation

  • This PR requires documentation updates when merged.

    • I will submit a noir-lang/docs PR.
    • I will request for and support Dev Rel's help in documenting this PR.

Additional Context

PR Checklist*

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

@TomAFrench TomAFrench changed the title chore: resolve instruction inputs fully before checking against cache chore: resolve Instruction inputs fully before checking against cache Aug 29, 2023
@jfecher
Copy link
Contributor

jfecher commented Aug 29, 2023

I'm not sure if there's a deeper issue in #2450 which wasn't caught which means we should revert that change. @jfecher what do you think?

@vezenovm and I did find a few bugs yesterday in that the DIE pass' definition of a side-effectful instruction was different than the definition needed by this pass to remove duplicates. allocate instructions within the same block were getting removed for example. A PR has been merged to fix that, although we found out later that load instructions to the same address were getting removed as well - you could try adding that case to see if it fixes your test. I have it locally as well in my mem2reg PRs.

@TomAFrench
Copy link
Member Author

It seems like Truncate is the culprit here but I can't understand why that would be the case as it's a relatively well-behaved instruction.

@vezenovm
Copy link
Contributor

vezenovm commented Aug 29, 2023 •
edited
Loading

It seems like Truncate is the culprit here but I can't understand why that would be the case as it's a relatively well-behaved instruction.

This PR maps the instruction inputs values, so perhaps there are two truncates on the same value where one instruction truncates to a smaller bit size. The smaller truncation comes second in line, so the value is replaced with values that have too large of a bit size.

Seems to be another case of the DIE pass' definition of an instruction with side-effects differs from that definition in this pass.

@TomAFrench
Copy link
Member Author

The target bitsize is part of the key on the cache though so I would expect that situation not to cause troubles. In any case, now we know it's truncation instructions I'll have a closer look at how these are being optimized out.

@TomAFrench TomAFrench marked this pull request as ready for review August 29, 2023 15:42
@TomAFrench
Copy link
Member Author

I'm happy to merge this without Truncate and then revisit this later.

@TomAFrench
Copy link
Member Author

I'd like to put this into a non-breaking release today before we start merging in the changes from acvm-backend-barretenberg.

jfecher
jfecher approved these changes Aug 29, 2023
View reviewed changes
Copy link
Contributor

@jfecher jfecher left a comment

Choose a reason for hiding this comment

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

is_pure is a good addition

@jfecher jfecher added this pull request to the merge queue Aug 29, 2023
Merged via the queue into master with commit 46dc894 Aug 29, 2023
16 checks passed
@jfecher jfecher deleted the fully-resolved-duplicated-instructions branch August 29, 2023 17:09
TomAFrench added a commit that referenced this pull request Aug 30, 2023
@TomAFrench
Merge branch 'master' into kw/bb-binaries
47f2ab6 
* master: (42 commits)
  fix(ssa): Handle right shift with constants (#2481)
  chore(noir): Release 0.10.4 (#2354)
  fix: Divide by zero should fail to satisfy constraints for `Field` and ints (#2475)
  fix(ssa): Remove padding from ToRadix call with constant inputs (#2479)
  fix: Implement handling of array aliasing in the mem2reg optimization pass (#2463)
  chore: resolve `Instruction` inputs fully before checking against cache (#2472)
  chore: Move independent `run_test` function into nargo core (#2468)
  feat: Standard library functions can now be called with closure args  (#2471)
  feat(frontend): aztec syntactic sugar (feature flagged) (#2403)
  chore(ci): enforce compliance with `cargo fmt` (#2467)
  chore(ci): Allow releases to have additional feature flags (#2405)
  feat: Add `assert_eq` keyword (#2137)
  fix(ssa): Do not optimize for allocates in constant folding (#2466)
  feat(ssa): Reuse existing results for duplicated instructions with no side-effects (#2460)
  fix: Closure lvalue capture bugfix (#2457)
  feat: Syntax for environment types now works with generics (#2383)
  fix(parser): fixes for the parsing of 'where' clauses (#2430)
  fix: Run `wasm` nodejs tests with no fails (#2387)
  chore: Run `cargo fmt` (#2455)
  chore: Perform formatting changes to integration tests (#2448)
  ...
TomAFrench added a commit that referenced this pull request Aug 30, 2023
@TomAFrench
Merge branch 'master' into remove-keys-from-artifact
b04ed6b 
* master: (42 commits)
  fix(ssa): Handle right shift with constants (#2481)
  chore(noir): Release 0.10.4 (#2354)
  fix: Divide by zero should fail to satisfy constraints for `Field` and ints (#2475)
  fix(ssa): Remove padding from ToRadix call with constant inputs (#2479)
  fix: Implement handling of array aliasing in the mem2reg optimization pass (#2463)
  chore: resolve `Instruction` inputs fully before checking against cache (#2472)
  chore: Move independent `run_test` function into nargo core (#2468)
  feat: Standard library functions can now be called with closure args  (#2471)
  feat(frontend): aztec syntactic sugar (feature flagged) (#2403)
  chore(ci): enforce compliance with `cargo fmt` (#2467)
  chore(ci): Allow releases to have additional feature flags (#2405)
  feat: Add `assert_eq` keyword (#2137)
  fix(ssa): Do not optimize for allocates in constant folding (#2466)
  feat(ssa): Reuse existing results for duplicated instructions with no side-effects (#2460)
  fix: Closure lvalue capture bugfix (#2457)
  feat: Syntax for environment types now works with generics (#2383)
  fix(parser): fixes for the parsing of 'where' clauses (#2430)
  fix: Run `wasm` nodejs tests with no fails (#2387)
  chore: Run `cargo fmt` (#2455)
  chore: Perform formatting changes to integration tests (#2448)
  ...
@TomAFrench TomAFrench mentioned this pull request Jan 4, 2024
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jfecher jfecher jfecher approved these changes

@vezenovm vezenovm Awaiting requested review from vezenovm

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

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