fix #582: GCC-compatible inline ARM64 ASM #585
Conversation
Summary of ChangesHello @mratsim, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a known GCC limitation concerning the number of register operands allowed in ARM64 inline assembly. The changes involve refactoring the assembly macros for Montgomery multiplication and reduction to reduce the overall register count. This ensures compatibility with GCC compilers while carefully considering and aiming to minimize any potential performance trade-offs related to instruction-level parallelism. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly addresses a GCC compiler limitation on ARM64 that restricts the number of register operands in inline assembly. The changes reduce the register count by removing temporary registers used for breaking dependency chains and by aliasing registers where possible. This is a necessary fix for GCC compatibility. You've correctly identified the potential performance trade-off (reduced instruction-level parallelism) and have documented it well in the code. The changes are consistent and look solid.
| bi = scratch[4] # Stores b[i] during mul and u during reduction | ||
| m = scratch[5] # Red step: (t[0] * m0ninv) mod 2ʷ | ||
| m = scratch[4] # Red step: (t[0] * m0ninv) mod 2ʷ |
There was a problem hiding this comment.
The comment for bi on line 298 appears to be outdated. It mentions that scratch[4] stores u during reduction, but u is now assigned to scratch[5]. Since scratch[4] is now reused for m, I suggest updating the comment for bi to reflect its actual usage and avoid potential confusion.
bi = scratch[4] # Stores b[i] during mul
m = scratch[4] # Red step: (t[0] * m0ninv) mod 2ʷ
|
New assembly 
It seems like there is a 4% performance impact on scalar multiplication (BLS signature) and 5.5% performance impact on pairing (BLS verification) |
|
It seems like we saved enough registers in the Montgomery multiplication but not in the fused sumproduct or the Montgomery reduction :/ 
|
See #582, downstream status-im/nimbus-eth1#3587 and #581.
GCC hardstops on current ARM64 inline ASM and cannot take full advantage of ARM hardware, which has 31 registers (+ the zero register) as it doesn't accept more than 30 register operands, with input/output registers being counted twice.
This reduces register usage count by 2 at the price of less instruction-level parallelism exposed. Hopefully impact is negligeable. Benchmarks pending.
cc @advaita-saha, etan-status