Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

clean: remove uneeded x86 asm and files #192

Merged
merged 18 commits into from
May 25, 2022
Merged

clean: remove uneeded x86 asm and files #192

merged 18 commits into from
May 25, 2022

Conversation

gbotrel
Copy link
Collaborator

@gbotrel gbotrel commented May 23, 2022 •
edited

  • remove unused fuzz test files
  • remove Add Sub Double Neg x86 asm impl --> latest versions of Go made them obsolete. Register based calling convention + inlining helps better, plus this reduces the assembly surface
  • fix: SetInterface(nil) panic
  • document Mul algorithm
  • clean up Inverse util functions to reduce un-needed code
  • fix: SetRandom uses crypto/rand.Int

@gbotrel
Copy link
Collaborator Author

gbotrel commented May 23, 2022

For bn254:

bn254 [develop] $ benchcmp asm.txt noasm.txt 
benchcmp is deprecated in favor of benchstat: https://pkg.go.dev/golang.org/x/perf/cmd/benchstat
benchmark                                     old ns/op     new ns/op     delta
BenchmarkG1JacScalarMul/double_and_add-16     81311         67143         -17.42%
BenchmarkG1JacScalarMul/double_and_add-16     78946         66939         -15.21%
BenchmarkG1JacScalarMul/GLV-16                50308         44839         -10.87%
BenchmarkG1JacScalarMul/GLV-16                50956         44846         -11.99%
BenchmarkPairing-16                           408676        367461        -10.09%
BenchmarkPairing-16                           418115        368043        -11.98%

For bls12-381:

benchmark                                     old ns/op     new ns/op     delta
BenchmarkG1JacScalarMul/double_and_add-16     118324        117431        -0.75%
BenchmarkG1JacScalarMul/double_and_add-16     118725        117927        -0.67%
BenchmarkG1JacScalarMul/GLV-16                77148         75270         -2.43%
BenchmarkG1JacScalarMul/GLV-16                76221         75447         -1.02%
BenchmarkPairing-16                           576880        573722        -0.55%
BenchmarkPairing-16                           574163        579537        +0.94%

@gbotrel gbotrel requested a review from Tabaie May 25, 2022 03:11
Tabaie
Tabaie approved these changes May 25, 2022
View reviewed changes
field/internal/templates/element/inverse.go
// If the function is constant-time ish, this loop will not run (no need to take it out explicitly)
for ; i < invIterationsN; i += 2 {
v.mulWSigned(&v, pSq)
// could optimize further with mul by word routine or by pre-computing a table since with k=26,
Copy link
Contributor

Choose a reason for hiding this comment

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

We used to have a precomputed table, but decided against it because of the added code complexity.

field/internal/templates/element/inverse.go Show resolved Hide resolved
field/internal/templates/element/conv.go Show resolved Hide resolved
@gbotrel gbotrel merged commit 7aa73d8 into develop May 25, 2022
@gbotrel gbotrel deleted the cleanup branch May 25, 2022 18:51
Tabaie
Tabaie reviewed May 25, 2022
View reviewed changes
Copy link
Contributor

@Tabaie Tabaie left a comment

Choose a reason for hiding this comment

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

LGTM

@gbotrel gbotrel mentioned this pull request Aug 3, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Tabaie Tabaie Tabaie approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@gbotrel @Tabaie