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

feat: create a subdirectory per mode in debug/ #11

Conversation

sayon
Copy link
Contributor

@sayon sayon commented Apr 13, 2024

What ❔

Merging this PR requires merging matter-labs/era-compiler-llvm-context#24 first.

Support creating subfolders in debug/.

Why ❔

Compiler tester overrides IR dumps if they were produced by launching a test repeatedly with different modes.

This change should not break behavior expected from era-compiler-llvm-context.

Initially I wanted to just encode modes in the filenames but it turned out to be very intrusive to the code base (for one, dumping .yul files is done by a very different code than dumping everything else). Therefore we are creating a subdirectory per mode.

@sayon
Copy link
Contributor Author

sayon commented Apr 13, 2024

Note: run CI after merging matter-labs/era-compiler-llvm-context#24 and before merging this PR.

@sayon sayon requested a review from hedgar2017 April 13, 2024 15:34
Copy link
Collaborator

@hedgar2017 hedgar2017 left a comment

Choose a reason for hiding this comment

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

Address the 2nd comment as well please.

@sayon sayon requested a review from hedgar2017 April 14, 2024 17:53
@hedgar2017
Copy link
Collaborator

@sayon thanks!
Please note that you have to do cargo update for zksolc and zkvyper as well, so they depend on the correct commit of era-compiler-llvm-context, and then do cargo update here in the tester so it depends on everything up-to-date.

@hedgar2017
Copy link
Collaborator

Address the 2nd comment as well please.

@sayon have you checked locally before commiting? There are compile errors.

Copy link

github-actions bot commented Apr 14, 2024

Benchmark results:

╔═╡ Size (-%) ╞════════════════╡ All M3B3 ╞═╗
║ Mean                                0.000 ║
║ Best                                0.000 ║
║ Worst                               0.000 ║
║ Total                               0.000 ║
╠═╡ Cycles (-%) ╞══════════════╡ All M3B3 ╞═╣
║ Mean                                0.000 ║
║ Best                                0.000 ║
║ Worst                               0.000 ║
║ Total                               0.000 ║
╠═╡ Ergs (-%) ╞════════════════╡ All M3B3 ╞═╣
║ Mean                                0.000 ║
║ Best                                0.000 ║
║ Worst                               0.000 ║
║ Total                               0.000 ║
╚═══════════════════════════════════════════╝

╔═╡ Size (-%) ╞════════════════╡ All MzB3 ╞═╗
║ Mean                                0.000 ║
║ Best                                0.000 ║
║ Worst                               0.000 ║
║ Total                               0.000 ║
╠═╡ Cycles (-%) ╞══════════════╡ All MzB3 ╞═╣
║ Mean                                0.000 ║
║ Best                                0.000 ║
║ Worst                               0.000 ║
║ Total                               0.000 ║
╠═╡ Ergs (-%) ╞════════════════╡ All MzB3 ╞═╣
║ Mean                                0.000 ║
║ Best                                0.000 ║
║ Worst                               0.000 ║
║ Total                               0.000 ║
╚═══════════════════════════════════════════╝

╔═╡ Size (-%) ╞════════╡ Precompiles M3B3 ╞═╗
║ Mean                                0.000 ║
║ Best                                0.000 ║
║ Worst                               0.000 ║
║ Total                               0.000 ║
╠═╡ Cycles (-%) ╞══════╡ Precompiles M3B3 ╞═╣
║ Mean                                0.000 ║
║ Best                                0.000 ║
║ Worst                               0.000 ║
║ Total                               0.000 ║
╠═╡ Ergs (-%) ╞════════╡ Precompiles M3B3 ╞═╣
║ Mean                                0.000 ║
║ Best                                0.000 ║
║ Worst                               0.000 ║
║ Total                               0.000 ║
╚═══════════════════════════════════════════╝

╔═╡ Size (-%) ╞════════╡ Precompiles MzB3 ╞═╗
║ Mean                                0.000 ║
║ Best                                0.000 ║
║ Worst                               0.000 ║
║ Total                               0.000 ║
╠═╡ Cycles (-%) ╞══════╡ Precompiles MzB3 ╞═╣
║ Mean                                0.000 ║
║ Best                                0.000 ║
║ Worst                               0.000 ║
║ Total                               0.000 ║
╠═╡ Ergs (-%) ╞════════╡ Precompiles MzB3 ╞═╣
║ Mean                                0.000 ║
║ Best                                0.000 ║
║ Worst                               0.000 ║
║ Total                               0.000 ║
╚═══════════════════════════════════════════╝

╔═╡ Size (-%) ╞══════════╡ Real life M3B3 ╞═╗
║ Mean                                0.000 ║
║ Best                                0.000 ║
║ Worst                               0.000 ║
║ Total                               0.000 ║
╠═╡ Cycles (-%) ╞════════╡ Real life M3B3 ╞═╣
║ Mean                                0.000 ║
║ Best                                0.000 ║
║ Worst                               0.000 ║
║ Total                               0.000 ║
╠═╡ Ergs (-%) ╞══════════╡ Real life M3B3 ╞═╣
║ Mean                                0.000 ║
║ Best                                0.000 ║
║ Worst                               0.000 ║
║ Total                               0.000 ║
╚═══════════════════════════════════════════╝

╔═╡ Size (-%) ╞══════════╡ Real life MzB3 ╞═╗
║ Mean                                0.000 ║
║ Best                                0.000 ║
║ Worst                               0.000 ║
║ Total                               0.000 ║
╠═╡ Cycles (-%) ╞════════╡ Real life MzB3 ╞═╣
║ Mean                                0.000 ║
║ Best                                0.000 ║
║ Worst                               0.000 ║
║ Total                               0.000 ║
╠═╡ Ergs (-%) ╞══════════╡ Real life MzB3 ╞═╣
║ Mean                                0.000 ║
║ Best                                0.000 ║
║ Worst                               0.000 ║
║ Total                               0.000 ║
╚═══════════════════════════════════════════╝

hedgar2017
hedgar2017 previously approved these changes Apr 14, 2024
Copy link
Collaborator

@hedgar2017 hedgar2017 left a comment

Choose a reason for hiding this comment

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

Can be some weird CI issue.
Let's see why benchmark works and default fails.

@sayon
Copy link
Contributor Author

sayon commented Apr 14, 2024

I see the problem! It worked indeed on my machine, but I was using a patch in Cargo.toml to override the dependency on llvm context.

Should I manually adjust the commit hash of era-compiler-llvm-context in Cargo.lock and commit it? Or do cargo update and commit it?

As for CI: Maybe CI does "cargo update" before launching tests? Therefore it has Cargo.lock with a newer dependency, not an obsolete reference to a previous commit.

@hedgar2017
Copy link
Collaborator

I see the problem! It worked indeed on my machine, but I was using a patch in Cargo.toml to override the dependency on llvm context.

Should I manually adjust the commit hash of era-compiler-llvm-context in Cargo.lock and commit it? Or do cargo update and commit it?

As for CI: Maybe CI does "cargo update" before launching tests? Therefore it has Cargo.lock with a newer dependency, not an obsolete reference to a previous commit.

No, CI shouldn't do it. Please do cargo update and push Cargo.lock!

@sayon sayon marked this pull request as draft April 15, 2024 14:23
@sayon sayon marked this pull request as ready for review April 15, 2024 17:03
Copy link
Collaborator

@hedgar2017 hedgar2017 left a comment

Choose a reason for hiding this comment

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

Apparently you're editing Cargo.lock manually. This is not a common practice as this file is supposed to be managed by cargo. You need to specify branches in Cargo.toml, do cargo update and push Cargo.lock.

Please read the cargo docs on this before proceeding.

@hedgar2017 hedgar2017 changed the title feature: create a subdirectory per mode in debug/ feat: create a subdirectory per mode in debug/ Apr 20, 2024
@sayon sayon force-pushed the iz-cpr-1657-make-compiler-tester-dump-ir-files-separately-for-each-mode branch from 5d687c2 to 5c6c515 Compare April 22, 2024 15:38
@hedgar2017 hedgar2017 merged commit bf43c6f into main Apr 22, 2024
6 checks passed
@hedgar2017 hedgar2017 deleted the iz-cpr-1657-make-compiler-tester-dump-ir-files-separately-for-each-mode branch April 22, 2024 16:18
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.

2 participants