Skip to content

Conversation

@jonathanpwang
Copy link
Contributor

found while reviewing previous PR

@jonathanpwang
Copy link
Contributor Author

jonathanpwang commented Jan 5, 2026

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes an x86-64 assembly bug in the AOT (ahead-of-time) compilation module for the RV32IM circuit. The bug incorrectly used a 32-bit memory operation (dword ptr) with a 4-byte stride when accessing elements of addr_space_access_count, which is actually an array of usize values (64-bit on x86-64 systems).

Key Changes:

  • Corrected assembly instruction from dword ptr with * 4 stride to qword ptr with * 8 stride to match the 64-bit usize element size

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Contributor

@nyunyunyunyu nyunyunyunyu left a comment

Choose a reason for hiding this comment

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

It's OK but maybe it's better to use u32 instead of usize(which size is not constant)

@github-actions

This comment has been minimized.

@codspeed-hq
Copy link

codspeed-hq bot commented Jan 5, 2026

CodSpeed Performance Report

Merging #2340 will not alter performance

Comparing fix/aot-update-boundary (8398a0b) with main (51dd038)

⚠️ Unknown Walltime execution environment detected

Using the Walltime instrument on standard Hosted Runners will lead to inconsistent data.

For the most accurate results, we recommend using CodSpeed Macro Runners: bare-metal machines fine-tuned for performance measurement consistency.

Summary

✅ 24 untouched
⏩ 36 skipped1

Footnotes

  1. 36 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@jonathanpwang
Copy link
Contributor Author

It's OK but maybe it's better to use u32 instead of usize(which size is not constant)

switched to u32

@github-actions
Copy link

github-actions bot commented Jan 5, 2026

group app.proof_time_ms app.cycles app.cells_used leaf.proof_time_ms leaf.cycles leaf.cells_used
verify_fibair (-55 [-19.2%]) 231 322,610 2,058,654 - - -
fibonacci (-25 [-2.4%]) 1,022 1,500,209 2,100,402 - - -
regex (+25 [+1.1%]) 2,354 4,137,502 17,695,216 - - -
ecrecover (-7 [-1.0%]) 727 122,859 2,262,772 - - -
pairing (+18 [+1.3%]) 1,443 1,745,742 25,408,302 - - -

Commit: 8398a0b

Benchmark Workflow

@jonathanpwang jonathanpwang merged commit 0f1b5fc into main Jan 5, 2026
70 checks passed
@jonathanpwang jonathanpwang deleted the fix/aot-update-boundary branch January 5, 2026 05:45
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.

3 participants