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] opcode offsets redesign #1242

Merged
merged 21 commits into from
Jan 21, 2025
Merged

[feat] opcode offsets redesign #1242

merged 21 commits into from
Jan 21, 2025

Conversation

Golovanov399
Copy link
Contributor

@Golovanov399 Golovanov399 commented Jan 20, 2025

This resolves INT-2355 to some extent.

While this doesn't eliminate the need for the offsets completely, this change presents an API for opcode mapping at the cost of need to define the new VmCoreAir trait function start_offset(&self) -> usize. After that one can use opcode_to_global_expr or expr_to_global_expr to map from local opcode to global in the air.

In most of the chips though the offset is no more stored and the default offset is used instead.

To convert from global to local, we can still use the VmOpcode::local_opcode_idx(offset) method.

Among other things:

  • UsizeOpcode is renamed to LocalOpcode
  • UsizeOpcode::default_offset() is now LocalOpcode::CLASS_OFFSET,
  • UsizeOpcode::as_usize() is now LocalOpcode::local_usize() to avoid confusion,
  • UsizeOpcode::with_default_offset() is now LocalOpcode::global_opcode(),
  • All of the above, including the LocalOpcode trait itself, can be renamed easily.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

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.

LGTM except:

  • some variables named class_offset should be start_offset
  • the loadstore core chip's usage of LocalOpcode as a generic differs from how it's handled in e.g., ALU. is there a reason or is this some leftover?

Copy link

group app.proof_time_ms app.cycles app.cells_used leaf.proof_time_ms leaf.cycles leaf.cells_used
verify_fibair (-31 [-1.3%]) 2,274 (-17268 [-3.4%]) 494,624 (-482045 [-2.5%]) 18,828,544 - - -
fibonacci_program (-66 [-1.1%]) 6,088 1,500,137 51,505,102 - - -
regex_program (-281 [-1.5%]) 18,685 4,190,904 165,028,173 - - -
ecrecover_program (-16 [-0.6%]) 2,605 285,401 15,092,297 - - -

Commit: f18dcb6

Benchmark Workflow

@Golovanov399 Golovanov399 merged commit 3a19b7a into main Jan 21, 2025
22 checks passed
@Golovanov399 Golovanov399 deleted the feat/opcode-trait branch January 21, 2025 19:56
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