Skip to content

Add MLD_CONFIG_MULTILEVEL_WITH_SHARED and NO_SHARED #532

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
Merged
merged 1 commit into from
Oct 9, 2025
Merged

Add MLD_CONFIG_MULTILEVEL_WITH_SHARED and NO_SHARED #532

merged 1 commit into from
Oct 9, 2025

Conversation

jakemas
Copy link
Contributor

@jakemas jakemas commented Oct 4, 2025 •
edited
Loading

TODO/FOLLOWUP:

@jakemas jakemas mentioned this pull request Oct 4, 2025
Merged
@jakemas jakemas marked this pull request as ready for review October 6, 2025 23:39
@jakemas jakemas requested a review from a team as a code owner October 6, 2025 23:39
@jakemas
Copy link
Contributor Author

jakemas commented Oct 6, 2025

Considering splitting in 2 parts. So far this PR lays down a good foundation for the new config. Hanno suggested first populating examples/ with the basic case (ala mlkem-native) so we have some infrastructure to test plumbing and configuration during the multi-level-build build-out. So I've pulled #533 to do exactly that. Happy to hear thoughts on how much to add to this PR before a merge.

mkannwischer
mkannwischer requested changes Oct 7, 2025
View reviewed changes
Copy link
Contributor

@mkannwischer mkannwischer left a comment

Choose a reason for hiding this comment

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

I don't see MLD_CONFIG_MULTILEVEL_WITH_SHARED / MLD_CONFIG_MULTILEVEL_NO_SHARED having any effect yet.
For example ntt.c should be guarded - poly.c should also be guarded, but that is more work.
I'm fine with merging this in smaller steps, but ntt.c definitely need to be guarded and a follow-up issue created for guarding poly.c.

@jakemas jakemas force-pushed the config-multilevel-build branch 2 times, most recently from b9bb021 to 455881b Compare October 7, 2025 23:27
@jakemas
Copy link
Contributor Author

jakemas commented Oct 7, 2025

I don't see MLD_CONFIG_MULTILEVEL_WITH_SHARED / MLD_CONFIG_MULTILEVEL_NO_SHARED having any effect yet. For example ntt.c should be guarded - poly.c should also be guarded, but that is more work. I'm fine with merging this in smaller steps, but ntt.c definitely need to be guarded and a follow-up issue created for guarding poly.c.

Okay, added use in ntt.c poly.c poly_kl.c debug.c and polyvec.c

mkannwischer
mkannwischer requested changes Oct 8, 2025
View reviewed changes
Copy link
Contributor

@mkannwischer mkannwischer left a comment

Choose a reason for hiding this comment

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

Thanks @jakemas! Getting there.
polyvec.c should not be guarded by MLD_CONFIG_MULTILEVEL_NO_SHARED - we actually need to build that one 3 times.
The comment in config.h still does not make sense.

Please create a follow-up issue to clean up poly_kl.c - that leads to a lot of duplicate code right now.

@jakemas jakemas force-pushed the config-multilevel-build branch from a425d02 to a776051 Compare October 8, 2025 06:59
@mkannwischer mkannwischer force-pushed the config-multilevel-build branch 3 times, most recently from c5501c6 to 5106771 Compare October 8, 2025 10:08
mkannwischer
mkannwischer approved these changes Oct 8, 2025
View reviewed changes
Copy link
Contributor

@mkannwischer mkannwischer left a comment

Choose a reason for hiding this comment

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

It remains to be seen if this PR actually works, but the changes look good to me. Let's get this merged and move forward in small steps.

I created a follow-up issue #536.

@jakemas @mkannwischer
Add MLD_CONFIG_MULTILEVEL_WITH_SHARED and NO_SHARED
54d1c4a 
This PR removes NAMESPACETOP and implements the NAMESPACE_KL
for multilevel functionality. Also added are the config
MLD_CONFIG_MULTILEVEL_WITH_SHARED and the no shared option
MLD_CONFIG_MULTILEVEL_NO_SHARED. These multilevel guards
have been added to ntt.c/polyvec.c/debug.c. The functions
within poly.c have been split into shared and parameter-
dependent in poly_kl.c.

Signed-off-by: Jake Massimo <[email protected]>
@mkannwischer mkannwischer force-pushed the config-multilevel-build branch from 5106771 to 54d1c4a Compare October 8, 2025 10:09
@jakemas
Copy link
Contributor Author

jakemas commented Oct 8, 2025

It remains to be seen if this PR actually works, but the changes look good to me. Let's get this merged and move forward in small steps.

I created a follow-up issue #536.

Good with me! Splitting up will be much easier to develop + review. Anything outstanding on this PR?

@jakemas jakemas mentioned this pull request Oct 8, 2025
Open
@mkannwischer mkannwischer merged commit d6f0aff into main Oct 9, 2025
243 checks passed
@mkannwischer mkannwischer deleted the config-multilevel-build branch October 9, 2025 00:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mkannwischer mkannwischer mkannwischer 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.

Add MLD_CONFIG_MULTILEVEL_WITH_SHARED and MLD_CONFIG_MULTILEVEL_NO_SHARED Remove MLD_NAMESPACETOP Split files into level-specific and level-generic

2 participants

@jakemas @mkannwischer