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

Unpacked API for ML-KEM #330

Merged
merged 39 commits into from
Jul 10, 2024
Merged

Unpacked API for ML-KEM #330

merged 39 commits into from
Jul 10, 2024

Conversation

karthikbhargavan
Copy link
Contributor

This PR adds an unpacked API for the three variants of ML-KEM (512, 768, 1024) and for the three platforms we support (avx2, neon, portable). There is no generic multiplexing API for this, since the unpacked API exposes internal state which is different for the three platforms.

@karthikbhargavan karthikbhargavan self-assigned this Jun 24, 2024
Copy link
Member

@franziskuskiefer franziskuskiefer left a comment

Choose a reason for hiding this comment

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

Please add documentation to the public APIs. You can also add a warn(missing_docs) to the crate to show the places. We need that anyway.

libcrux-ml-kem/src/types.rs Show resolved Hide resolved
libcrux-ml-kem/src/mlkem1024.rs Show resolved Hide resolved
@franziskuskiefer
Copy link
Member

df8b66e is not the change that will make this work. The abstraction is required for the extraction, and exposing internal traits is generally a bad idea.
To make it work, you need to add the bounds to all types, as done 768. That way C extraction goes through, you can suppress the Rust warning until we have fixed the tools.

@karthikbhargavan
Copy link
Contributor Author

Thanks, will aim to address all of these by the weekend.

@karthikbhargavan
Copy link
Contributor Author

I think I fixed everything, but still need to merge the C code conflicts.

@karthikbhargavan
Copy link
Contributor Author

Merged, resolved conflicts, regenerated C code.

@karthikbhargavan
Copy link
Contributor Author

Please add documentation to the public APIs. You can also add a warn(missing_docs) to the crate to show the places. We need that anyway.

Done

@github-actions github-actions bot dismissed stale reviews from franziskuskiefer and franziskuskiefer July 10, 2024 01:24

Review re-requested

Copy link
Member

@franziskuskiefer franziskuskiefer left a comment

Choose a reason for hiding this comment

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

Thanks! lgtm with a few final nits.

Looks like there are a mlkem768.cc~ files that shouldn't be there.

libcrux-ml-kem/src/ind_cca.rs Show resolved Hide resolved
libcrux-ml-kem/src/types.rs Show resolved Hide resolved
libcrux-ml-kem/src/types.rs Outdated Show resolved Hide resolved
libcrux-ml-kem/src/mlkem512.rs Outdated Show resolved Hide resolved
libcrux-ml-kem/benches/ml-kem.rs Show resolved Hide resolved
Copy link
Member

@franziskuskiefer franziskuskiefer left a comment

Choose a reason for hiding this comment

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

:shipit:

@karthikbhargavan karthikbhargavan added this pull request to the merge queue Jul 10, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 10, 2024
@franziskuskiefer franziskuskiefer merged commit 0f6f248 into main Jul 10, 2024
49 checks passed
@franziskuskiefer franziskuskiefer deleted the karthik/unpacked-api branch July 10, 2024 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

4 participants