Skip to content

fix(aot): x86 assembly for addr_space_access_count update#2340

Merged
jonathanpwang merged 3 commits into
mainfrom
fix/aot-update-boundary
Jan 5, 2026
Merged

fix(aot): x86 assembly for addr_space_access_count update#2340
jonathanpwang merged 3 commits into
mainfrom
fix/aot-update-boundary

Conversation

@jonathanpwang

Copy link
Copy Markdown
Contributor

found while reviewing previous PR

@jonathanpwang

jonathanpwang commented Jan 5, 2026

Copy link
Copy Markdown
Contributor Author

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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.

@nyunyunyunyu nyunyunyunyu left a comment

Copy link
Copy Markdown
Contributor

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

codspeed-hq Bot commented Jan 5, 2026

Copy link
Copy Markdown

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
Copy Markdown
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

github-actions Bot commented Jan 5, 2026

Copy link
Copy Markdown
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