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

Parameters Performance Improvements #177

Merged
merged 2 commits into from
Nov 1, 2023
Merged

Conversation

bcdonovan
Copy link
Contributor

This PR adds performance improvements for working with parameters.

Changes to MergeCircuitPass:

  • Remove some unused vectors
  • Change circuit merge so that only unique operands / arguments are added when merging circuits rather than the union of the two circuits arguments.

Changes to ParameterInitialValueAnalysis:

  • Change internal variables to be static
  • Modify pass so that it may be instantiated multiple times without invalidating / recomputing results. This is valid as the parameter initial values should not change during program analysis.

@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

Comment on lines 43 to 44
static InitialValueType initial_values_;
static bool invalid_;
Copy link
Collaborator

Choose a reason for hiding this comment

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

To avoid the usage of statics could we use the analysis system and preserve the analysis? https://mlir.llvm.org/docs/PassManagement/#preserving-analyses

@bcdonovan bcdonovan force-pushed the bd-performance-regression branch from 7dad980 to a54ff88 Compare November 1, 2023 01:07
@bcdonovan bcdonovan force-pushed the bd-performance-regression branch from a54ff88 to 4ce6d31 Compare November 1, 2023 01:20
Copy link
Contributor

github-actions bot commented Nov 1, 2023

clang-tidy review says "All clean, LGTM! 👍"

Copy link
Contributor

github-actions bot commented Nov 1, 2023

clang-tidy review says "All clean, LGTM! 👍"

1 similar comment
Copy link
Contributor

github-actions bot commented Nov 1, 2023

clang-tidy review says "All clean, LGTM! 👍"

Copy link
Contributor

@mbhealy mbhealy left a comment

Choose a reason for hiding this comment

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

Great work!

Copy link
Collaborator

@taalexander taalexander left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for finding a way around the statics 🙏

@bcdonovan bcdonovan merged commit 05d9d05 into main Nov 1, 2023
2 checks passed
@bcdonovan bcdonovan deleted the bd-performance-regression branch November 1, 2023 14:22
bcdonovan added a commit that referenced this pull request Nov 1, 2023
This PR adds performance improvements for working with parameters. 

Changes to MergeCircuitPass:
* Remove some unused vectors
* Change circuit merge so that only unique operands / arguments are
added when merging circuits rather than the union of the two circuits
arguments.
bcdonovan added a commit that referenced this pull request Nov 1, 2023
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.

3 participants