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

Normal format for custom_insn_* macros we have #1193

Merged
merged 7 commits into from
Jan 9, 2025

Conversation

Golovanov399
Copy link
Contributor

@Golovanov399 Golovanov399 commented Jan 8, 2025

This resolves INT-2667.

The following is done here:

  • custom_insn_r and custom_insn_i macros (are now procedural and) now have a clearer signature, where all arguments must be named, and the register types also have to be explicitly specified,
  • technically, these macros generate the code under the #[cfg(target_os = "zkvm")] block, which would help the developer to catch the wrong macro signature things instantly if they have the language server running, but in reality we also want to let uninit = ... under the same block anyway, so now this change is not really represented. Maybe we will want to make uninit an additional valid macro argument at some point, but this would overcomplicate things imo.

@Golovanov399 Golovanov399 force-pushed the feat/less-cursed-custom-insn-2 branch from 92635b2 to 76f4a47 Compare January 8, 2025 12:53
@Golovanov399 Golovanov399 force-pushed the feat/less-cursed-custom-insn-2 branch from 76f4a47 to 23cc1c0 Compare January 8, 2025 15:57

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

…) for less space for the wrong signature error

This comment has been minimized.

Copy link
Contributor

@jonathanpwang jonathanpwang left a comment

Choose a reason for hiding this comment

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

Overall this is a big improvement!
LGTM, but just the macro use of opcode = const #opcode to insert assembly is a little weird to me.
There is one const vs sym question.

Also, does anything in the book/docs need to be updated, or do we not even mention this macro in the book / documentation?

Heads up I will likely merge #1107 first, so there will be a small(hopefully) rebase necessary.

This comment has been minimized.

Copy link

github-actions bot commented Jan 9, 2025

group app.proof_time_ms app.cycles app.cells_used leaf.proof_time_ms leaf.cycles leaf.cells_used
verify_fibair (-15 [-0.4%]) 3,822 746,050 30,008,524 - - -
fibonacci_program (+48 [+0.8%]) 6,077 1,500,137 51,505,102 - - -
regex_program (+683 [+3.7%]) 19,197 4,190,904 165,028,173 - - -
ecrecover_program (+17 [+0.6%]) 2,645 285,401 15,092,297 - - -

Commit: 5ab6d6b

Benchmark Workflow

@Golovanov399
Copy link
Contributor Author

Btw yes, I think we don't mention these macros anywhere in the docs

@Golovanov399 Golovanov399 merged commit 928ab04 into main Jan 9, 2025
17 checks passed
@Golovanov399 Golovanov399 deleted the feat/less-cursed-custom-insn-2 branch January 9, 2025 17:17
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