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

transpose kernel in vec_ops and rust binding #462

Merged
merged 7 commits into from
Apr 9, 2024

Conversation

vladfdp
Copy link
Contributor

@vladfdp vladfdp commented Apr 7, 2024

Describe the changes

This PR adds an extern C link to the transpose kernel, now in vec_ops.cu.
Also Rust binding, and I updated the test check_ntt_batch to use the new transpose function.
The test passes.

Linked Issues

Resolves #

Copy link
Contributor

@ChickenLover ChickenLover left a comment

Choose a reason for hiding this comment

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

Congrats with your first PR @vladfdp! Excellent work

I left some minor comments. Also it's generally a good idea to add a test for any new functionally. Take a look at this test for example. You can add something like this for the transpose function

icicle/utils/vec_ops.cu Outdated Show resolved Hide resolved
icicle/utils/vec_ops.cu Outdated Show resolved Hide resolved
* @return `cudaSuccess` if the execution was successful and an error code otherwise.
*/
extern "C" cudaError_t CONCAT_EXPAND(CURVE, TransposeBatch)(
curve_config::scalar_t* input,
Copy link
Collaborator

Choose a reason for hiding this comment

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

better make the input const pointer (const scalar*). This is also true for the kernels.
In some cases (for example polynomials) the internal memory can only be accessed by const so this way means this API cannot be used.

@LeonHibnik LeonHibnik merged commit 4a35eec into main Apr 9, 2024
21 checks passed
@LeonHibnik LeonHibnik deleted the feat/vlad/transpose-api branch April 9, 2024 05:47
LeonHibnik added a commit that referenced this pull request Apr 9, 2024
## Describe the changes

This PR adds an extern C link to the transpose kernel, now in
vec_ops.cu.
Also Rust binding, and I updated the test check_ntt_batch to use the new
transpose function.
The test passes.

## Linked Issues

Resolves #

---------

Co-authored-by: LeonHibnik <leon@ingonyama.com>
yshekel pushed a commit that referenced this pull request May 19, 2024
## Describe the changes

This PR adds an extern C link to the transpose kernel, now in
vec_ops.cu.
Also Rust binding, and I updated the test check_ntt_batch to use the new
transpose function.
The test passes.

## Linked Issues

Resolves #

---------

Co-authored-by: LeonHibnik <leon@ingonyama.com>
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.

None yet

4 participants