chore: ecc/fields ASM audit#20892
Merged
Merged
Conversation
notnotraju
commented
Feb 26, 2026
| * @note the 2p is because for 254 bit fields, we use the _coarse_ representation. | ||
| **/ | ||
| #define ADD_REDUCE(b, modulus_0, modulus_1, modulus_2, modulus_3) \ | ||
| #define ADD_REDUCE(b, twice_not_modulus_0, twice_not_modulus_1, twice_not_modulus_2, twice_not_modulus_3) \ |
Contributor
Author
There was a problem hiding this comment.
only called with -2p
notnotraju
commented
Feb 26, 2026
| * 2. `asm_sub_with_coarse_reduction`: b = 2p. If r wrapped negative, return r + 2p. | ||
| **/ | ||
| #define REDUCE_FIELD_ELEMENT(neg_modulus_0, neg_modulus_1, neg_modulus_2, neg_modulus_3) \ | ||
| #define CONDITIONAL_ADD(b_0, b_1, b_2, b_3) \ |
Contributor
Author
There was a problem hiding this comment.
naming was confusing, as it wasn't MERELY used to do a single reduce, but also to conditionally add 2p in another method.
notnotraju
changed the title
ecc/fields ASM auditecc/fields ASM audit
notnotraju
added
the
bberg-int-audit
notnotraju
commented
Mar 11, 2026
| **/ | ||
| #define MUL_FOO(a1, a2, a3, a4, b) \ |
notnotraju
commented
Mar 11, 2026
| **/ | ||
| #define SQR(a) \ |
Contributor
Author
There was a problem hiding this comment.
as explained above, this was unused due to a rare bug.
iakovenkos
reviewed
Mar 11, 2026
| // 3: we replace `(p - *this).reduce_once()` with an assembly block that is equivalent to `p - *this`, | ||
| // but we call `REDUCE_FIELD_ELEMENT` with `not_twice_modulus` instead of `twice_modulus` | ||
| // but we call `CONDITIONAL_ADD` with `not_twice_modulus` instead of `twice_modulus` | ||
| // not sure which is faster and whether any of the above might break something! |
Contributor
There was a problem hiding this comment.
This comment seemingly needs to be more conclusive
Contributor
Author
There was a problem hiding this comment.
oh, yeah -- let me figure out what to do with that TODO, thanks!
Contributor
Author
There was a problem hiding this comment.
OK, figured out what we were doing and added some comments. Definitely a micro-optimization...
iakovenkos
approved these changes
Mar 11, 2026
added 6 commits
March 12, 2026 10:55
…UCE_FIELD_ELEMENT to CONDITIONAL_ADD, as it is not *only* used to with argument -p.
Fix systematic comment errors across SQR, ADX MUL, and both MUL_256 macros: - wrong k variable indices in reduction comments (e.g. k = r[9] instead of r[0]) - wrong modulus index comments (e.g. modulus[2] on mulxq %[modulus_3]) - swapped flag_c/flag_o on adcxq/adoxq instructions - flag_i typo - addq comments incorrectly claiming + flag_c (addq does not consume carry) - reused temp names across consecutive mulxq pairs (t[0],t[1] used twice) - wrong temp indices in downstream add comments - "move r_inv into %rdx" instead of "move r[3] into %rdx" - "(%rsi now free)" instead of "(%r10 now free)" Add register convention block comments and per-round register dictionaries to ADX MUL (matching the style already applied to non-ADX MUL). Remove MUL_FOO: identical copy of ADX MUL, never referenced anywhere.
notnotraju
force-pushed
the
rk/ecc-field-asm-1
branch
from
1bf5055 to
0843c49
Compare
github-merge-queue Bot
pushed a commit
that referenced
this pull request
Mar 12, 2026
BEGIN_COMMIT_OVERRIDE chore: unify tests in ecc/curves and ecc/groups (#21392) chore: `ecc/fields` ASM audit (#20892) chore: audit the `ecc/field` extension field (#21409) chore: responding to external IPA audit (#20334) feat: committed sumcheck for batched Hiding Translator (#21376) chore: documentation fixes for ECCVM after first external audit (#20348) chore: Document RAM gate cost optimization opportunity (#21426) refactor: rename MSGPACK_FIELDS to SERIALIZATION_FIELDS (#21175) END_COMMIT_OVERRIDE
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.