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.

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

perf!: use icicle msm #426

Merged
merged 16 commits into from
Jul 4, 2024
Merged

perf!: use icicle msm #426

merged 16 commits into from
Jul 4, 2024

Conversation

chokobole
Copy link
Contributor

@chokobole chokobole commented May 29, 2024

Description

This PR integrates icicle MSM for improved performance!

Previously, the MSM GPU interface accepted input directly from GPU memory, which complicated the use of MSM GPU for the KZG commitment scheme. While we could implement this for previous algorithms, such as Bellman MSM and CUZK, they were slow when dealing with non-random scalar distributions. The advantages of using icicle are twofold: (1) it supports input from CPU memory, and (2) it performs well even with non-random scalar distributions. Consequently, this PR removes the previous implementations and adopts icicle exclusively.

@chokobole chokobole changed the title perf: use icicle msm perf!: use icicle msm May 30, 2024
@chokobole chokobole marked this pull request as ready for review May 30, 2024 03:49
@chokobole chokobole marked this pull request as draft May 31, 2024 08:27
@chokobole chokobole force-pushed the perf/use-icicle-msm branch 3 times, most recently from 1937640 to 9fb43f1 Compare July 2, 2024 04:28
@chokobole chokobole marked this pull request as ready for review July 2, 2024 04:28
@chokobole chokobole requested review from GideokKim and removed request for Insun35 July 3, 2024 07:43
Copy link
Contributor

@TomTaehoonKim TomTaehoonKim left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@ashjeong ashjeong left a comment

Choose a reason for hiding this comment

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

Could you rebase to remove the first 5 commits?

c300a70
Please change to “…different types of commitments”

tachyon/c/math/elliptic_curves/generator/msm_gpu.h.tpl Outdated Show resolved Hide resolved
tachyon/c/math/elliptic_curves/msm/BUILD.bazel Outdated Show resolved Hide resolved
@TomTaehoonKim
Copy link
Contributor

How about adding GPU benchmark as well?

Copy link
Contributor

@GideokKim GideokKim left a comment

Choose a reason for hiding this comment

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

First of all, I only reviewed the Bazel part related to icicle.

third_party/icicle/icicle.BUILD Outdated Show resolved Hide resolved
third_party/icicle/icicle.BUILD Show resolved Hide resolved
third_party/icicle/icicle.BUILD Outdated Show resolved Hide resolved
third_party/icicle/icicle.BUILD Outdated Show resolved Hide resolved
Copy link
Contributor

@ashjeong ashjeong left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@dongchangYoo dongchangYoo left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@GideokKim GideokKim left a comment

Choose a reason for hiding this comment

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

LGTM

@chokobole chokobole merged commit a1b2d6e into main Jul 4, 2024
3 checks passed
@chokobole chokobole deleted the perf/use-icicle-msm branch July 4, 2024 10:17
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.

5 participants