Skip to content

feat: make bitReverse generic#736

Merged
gbotrel merged 15 commits intomasterfrom
perf/e4fft
Sep 3, 2025
Merged

feat: make bitReverse generic#736
gbotrel merged 15 commits intomasterfrom
perf/e4fft

Conversation

@gbotrel
Copy link
Copy Markdown
Collaborator

@gbotrel gbotrel commented Sep 2, 2025

Description

BitReverse is now generic. With tweaks, benchmarks are actually slightly faster (tested on c7a.8xlarge for koalabear and bls12377),, and faster for most sizes on koalabear E4:

BitReverse/name=bitReverseNaive/size=262144-32            1.121m ±  1%   1.015m ± 1%   -9.46% (p=0.000 n=10)
BitReverse/name=BitReverse/size=262144-32                 1.134m ±  1%   1.032m ± 1%   -8.97% (p=0.000 n=10)
BitReverse/name=bitReverseCobraInPlace/size=262144-32     3.489m ±  1%   3.228m ± 1%   -7.47% (p=0.000 n=10)
BitReverse/name=bitReverseNaive/size=524288-32            3.146m ±  3%   2.796m ± 1%  -11.13% (p=0.000 n=10)
BitReverse/name=BitReverse/size=524288-32                 3.145m ±  2%   2.747m ± 1%  -12.68% (p=0.000 n=10)
BitReverse/name=bitReverseCobraInPlace/size=524288-32     8.012m ± 12%   7.128m ± 2%  -11.04% (p=0.000 n=10)
BitReverse/name=bitReverseNaive/size=1048576-32           13.46m ±  2%   11.96m ± 1%  -11.16% (p=0.000 n=10)
BitReverse/name=BitReverse/size=1048576-32                13.28m ±  2%   11.74m ± 2%  -11.61% (p=0.000 n=10)
BitReverse/name=bitReverseCobraInPlace/size=1048576-32    17.15m ±  6%   14.96m ± 9%  -12.79% (p=0.000 n=10)
BitReverse/name=bitReverseNaive/size=2097152-32           46.55m ±  3%   43.40m ± 2%   -6.77% (p=0.000 n=10)
BitReverse/name=BitReverse/size=2097152-32                28.14m ±  5%   26.57m ± 6%   -5.57% (p=0.043 n=10)
BitReverse/name=bitReverseCobraInPlace/size=2097152-32    31.77m ±  4%   31.31m ± 3%   -1.46% (p=0.029 n=10)
BitReverse/name=bitReverseNaive/size=4194304-32          104.83m ±  1%   96.82m ± 3%   -7.64% (p=0.000 n=10)
BitReverse/name=BitReverse/size=4194304-32                57.14m ±  2%   55.12m ± 4%   -3.54% (p=0.004 n=10)
BitReverse/name=bitReverseCobraInPlace/size=4194304-32    66.32m ±  2%   61.54m ± 3%   -7.21% (p=0.000 n=10)
BitReverse/name=bitReverseNaive/size=8388608-32           221.3m ±  2%   188.0m ± 2%  -15.06% (p=0.000 n=10)
BitReverse/name=BitReverse/size=8388608-32                115.6m ±  3%   108.9m ± 2%   -5.79% (p=0.000 n=10)
BitReverse/name=bitReverseCobraInPlace/size=8388608-32    139.1m ±  2%   126.3m ± 2%   -9.24% (p=0.000 n=10)

Copy link
Copy Markdown
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 makes the BitReverse function generic by moving it from field-specific packages to a centralized utils package, allowing it to work with any type while maintaining performance optimizations. The function now accepts func BitReverse[T any](v []T) instead of being tied to specific field element types.

  • Move BitReverse implementation from field-specific fft packages to utils/bitreverse.go as a generic function
  • Update all references throughout the codebase to use utils.BitReverse instead of fft.BitReverse
  • Remove field-specific bitreverse.go and bitreverse_test.go files that are no longer needed

Reviewed Changes

Copilot reviewed 125 out of 125 changed files in this pull request and generated 4 comments.

File Description
utils/bitreverse.go New generic BitReverse implementation with performance optimizations
utils/bitreverse_test.go Comprehensive tests for the generic BitReverse function
Multiple fft packages Removal of field-specific bitreverse implementations
Multiple files Updated import statements and function calls to use utils.BitReverse

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

YaoJGalteland
YaoJGalteland previously approved these changes Sep 3, 2025
Base automatically changed from perf/e4vec to master September 3, 2025 20:24
@gbotrel gbotrel dismissed YaoJGalteland’s stale review September 3, 2025 20:24

The base branch was changed.

@gbotrel gbotrel merged commit a3f93ab into master Sep 3, 2025
6 checks passed
@gbotrel gbotrel deleted the perf/e4fft branch September 3, 2025 20:29
} else {
bitReverseNaive(v)
}
bitReverseCobraInPlace(v)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bug: BitReverse Function Fallback Issue

The bitReverseCobra function's default case no longer falls back to bitReverseNaive for small input sizes. This makes the len(v) < (1<<21) check in the top-level BitReverse function essential. Without it, small inputs would incorrectly use bitReverseCobraInPlace, which may lead to performance regressions.

Additional Locations (1)

Fix in Cursor Fix in Web

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