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: add ISA specific setting instrument_inst for zkASM #230

Merged
merged 5 commits into from
Feb 26, 2024

Conversation

mooori
Copy link

@mooori mooori commented Feb 23, 2024

  • Adds the ISA specific instrument_inst setting for zkasm.
  • Adds ZkasmSettings to handle ISA specific settings in zkasm_codegen.

Can be reviewed commit by commit and commit messages provide a summary of the changes.

Context

Benchmarking infra will use the instrument_inst setting to enable instruction tracing. I propose to add this in a separate PR.

Enabling instrument_inst

It is disabled by default. To see the changes introduced by this PR in action, it can be enabled with the following patch:

diff --git a/cranelift/filetests/src/test_zkasm.rs b/cranelift/filetests/src/test_zkasm.rs
index 63086e228..6ac7818ec 100644
--- a/cranelift/filetests/src/test_zkasm.rs
+++ b/cranelift/filetests/src/test_zkasm.rs
@@ -60,7 +60,8 @@ mod tests {
 
     fn test_module(name: &str) {
         let module_binary = wat::parse_file(format!("../zkasm_data/{name}.wat")).unwrap();
-        let settings = ZkasmSettings::default();
+        let mut settings = ZkasmSettings::default();
+        settings.instrument_inst = true;
         let program = zkasm_codegen::generate_zkasm(&settings, &module_binary);
         let expected =
             expect_test::expect_file![format!("../../zkasm_data/generated/{name}.zkasm")];

Then instrumented zkasm can be generated by running a test like:

env UPDATE_EXPECT=1 cargo test --package cranelift-filetests --lib -- test_zkasm::tests::add --nocapture

@mooori mooori marked this pull request as ready for review February 23, 2024 11:07
Copy link

@aborg-dev aborg-dev left a comment

Choose a reason for hiding this comment

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

Overall, looks good to me!

I would probably go with a more specific name for the option like emit_profiling_info as the current instrument_inst does not clearly show the purpose without reading the documentation.

@@ -63,6 +74,15 @@ pub fn generate_zkasm(wasm_module: &[u8]) -> String {
program.join("\n")
}

fn handle_zkasm_settings(
Copy link

@aborg-dev aborg-dev Feb 23, 2024

Choose a reason for hiding this comment

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

(just an opinion, no expectation to change anything)
I see the reasoning for having a dedicated function if we have more settings in the future, but at this stage, I would call this a premature optimization and follow YAGNI (You aren't gonna need it) [1], [2].

[1] https://en.wikipedia.org/wiki/You_aren%27t_gonna_need_it
[2] https://martinfowler.com/bliki/Yagni.html

Copy link
Author

Choose a reason for hiding this comment

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

Adding a separate handle_zkasm_settings function also aims at encapsulating functionality and avoiding that generate_zkasm(...) grows too big. I think the separate function does not add any significant overhead here. No strong opinion on my side, merging now to wrap up the PR.

@mooori mooori added this pull request to the merge queue Feb 26, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 26, 2024
@mooori mooori added this pull request to the merge queue Feb 26, 2024
Merged via the queue into near:main with commit a6ef91e Feb 26, 2024
20 checks passed
@mooori mooori deleted the compile-flags branch February 26, 2024 11:36
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.

None yet

3 participants