Skip to content
This repository has been archived by the owner on Apr 9, 2024. It is now read-only.

feat: Add range opcode optimization #219

Merged
merged 6 commits into from
Apr 20, 2023
Merged

feat: Add range opcode optimization #219

merged 6 commits into from
Apr 20, 2023

Conversation

kevaundray
Copy link
Contributor

Related issue(s)

Resolves #218

(If it does not already exist, first create a GitHub issue that describes the problem this Pull Request (PR) solves before creating the PR and link it here.)

Resolves (link to issue)

Description

Summary of changes

(Describe the changes in this PR. Point out breaking changes if any.)

Dependency additions / changes

(If applicable.)

Test additions / changes

(If applicable.)

Checklist

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt with default settings.
  • I have linked this PR to the issue(s) that it resolves.
  • I have reviewed the changes on GitHub, line by line.
  • I have ensured all changes are covered in the description.

Additional context

(If applicable.)

Copy link
Member

@TomAFrench TomAFrench left a comment

Choose a reason for hiding this comment

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

Generally looks good.

If we're being lazy about this rather than having the two passes be

  1. Build a mapping from witness to strictest num_bits.
  2. Iterate through opcodes, get minimal num_bits, check opcode hasn't already been added for witness already, retain opcode and mark witness as seen

We could change it to be:

  1. Build a mapping from witness to range opcode index with strictest num_bits.
    1.a. convert mapping to a HashSet of opcode indices to retain.
  2. Iterate through opcode discarding any range opcodes with indices not in the set.

The benefits probably aren't worth the extra time to change however.

acvm/src/compiler/optimizers/redundant_range.rs Outdated Show resolved Hide resolved
acvm/src/compiler/optimizers/mod.rs Outdated Show resolved Hide resolved
acvm/src/compiler/optimizers/redundant_range.rs Outdated Show resolved Hide resolved
@kevaundray
Copy link
Contributor Author

Generally looks good.

If we're being lazy about this rather than having the two passes be

  1. Build a mapping from witness to strictest num_bits.
  2. Iterate through opcodes, get minimal num_bits, check opcode hasn't already been added for witness already, retain opcode and mark witness as seen

We could change it to be:

  1. Build a mapping from witness to range opcode index with strictest num_bits.
    1.a. convert mapping to a HashSet of opcode indices to retain.
  2. Iterate through opcode discarding any range opcodes with indices not in the set.

The benefits probably aren't worth the extra time to change however.

Yeah I was thinking of being smart by not doing a double pass, but I just went with the simplest version now and then if we see it as a bottle-neck, can change to a more optimized version

@kevaundray kevaundray added this pull request to the merge queue Apr 20, 2023
Merged via the queue into master with commit 7abe6e5 Apr 20, 2023
@github-actions github-actions bot mentioned this pull request Apr 20, 2023
@TomAFrench
Copy link
Member

Yeah I was thinking of being smart by not doing a double pass

Actually yeah, we don't need to do a double pass at all. I was thinking you were going through the effort of not just bunging the range opcodes at the end as ideally we maintain the ordering of opcodes (as this minimises loops through the circuit looking for solvable opcodes) but range opcodes don't have any outputs so this isn't a concern.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Range gate optimization
2 participants