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

Benchmark compilation time for cancel_inverses and merge_rotations, catalyst-vs-PL #1207

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

paul0403
Copy link
Contributor

@paul0403 paul0403 commented Oct 15, 2024

Context:
Add a timer to cancel inverse and merge rotation passes for compile time measurements.

https://app.shortcut.com/xanaduai/epic/70803


Branch usage:

python3 peephole_benchmark_driver.py

Copy link
Contributor

Hello. You may have forgotten to update the changelog!
Please edit doc/releases/changelog-dev.md on your branch with:

  • A one-to-two sentence description of the change. You may include a small working example for new features.
  • A link back to this PR.
  • Your name (or GitHub username) in the contributors section.

@paul0403 paul0403 marked this pull request as draft October 15, 2024 21:04
@paul0403
Copy link
Contributor Author

drafting this --- no need to waste CI on this

@paul0403 paul0403 changed the title Add timer to cancel inverse pass Add timer to cancel inverse and merge rotation pass Oct 15, 2024
@paul0403 paul0403 changed the title Add timer to cancel inverse and merge rotation pass Add timer to cancel inverse and merge rotation passes Oct 15, 2024

auto stop = std::chrono::high_resolution_clock::now();
auto duration = std::chrono::duration_cast<std::chrono::microseconds>(stop - start);
llvm::errs() << "merge rotation pass runtime: " << duration.count() << " microseconds\n";
Copy link
Member

Choose a reason for hiding this comment

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

@paul0403 quick question, I saw I was tagged for review but I assume this branch is not going to be merged into main? 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No need to review, this is just for visibility, or if others want to use the compile time timing/plotting script (which I will add)

@dime10
Copy link
Collaborator

dime10 commented Oct 15, 2024

There is no need for bespoke timing code, that's what the instrumentation is for :)

@@ -323,6 +332,7 @@ def run_writing_command(command: List[str], compile_options: Optional[CompileOpt

DEFAULT_ASYNC_PIPELINES = [
ENFORCE_RUNTIME_INVARIANTS_PASS,
PEEPHOLE_BENCHMARK_PASS,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

irrelevant, as async is not used in the benchmark

@paul0403 paul0403 changed the title Add timer to cancel inverse and merge rotation passes Benchmark compilation time for cancel_inverses and merge_rotations, catalyst-vs-PL Oct 16, 2024
def run_one_circuit(timings, core_PL_timings, num_of_iters):
do(f"python3 my_toy_circuit.py {num_of_iters}")

with open('my_toy_circuit.yml', 'r') as f:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh no, you can use a yaml package to parse it for you and access the elements like dictionaries, lists, etc :)

@joeycarter
Copy link
Contributor

Might want to run black on the python files to get rid of all those codefactor warnings 😉

@paul0403
Copy link
Contributor Author

Please forgive my abhorrent code quality in these scripts (which are only supposed to be used by me) : )))))

print(loopsizes, walltimes, cputimes, programsizes, core_PL_times)


plt.figure(figsize=(15, 9))
Copy link
Contributor

Choose a reason for hiding this comment

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

One thing we may want to consider when harmonizing our plot styles is to use a consistent figsize, since that will change the size of the text relative to the rest of the figure when pasting these plots into slides or a document, for example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I actually had this in mind (hence why I chose the same colors as your plots) but didn't think about the font!

Let's talk about it when the data for all three plots are locked down. It also depends on the specs of the final presentation, etc.

cc @josh146

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

Successfully merging this pull request may close these issues.

4 participants