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

hotfix for slow twiddles on small domain sizes #557

Open
wants to merge 4 commits into
base: V2
Choose a base branch
from

Conversation

vhnatyk
Copy link
Contributor

@vhnatyk vhnatyk commented Jul 18, 2024

Describe the changes

This PR is a hotfix for ntt domains of log2 sizes < 4

Linked Issues

Resolves #

@vhnatyk
Copy link
Contributor Author

vhnatyk commented Jul 18, 2024

"proper fix" - the fast twiddles must only be used with mixed radix ntt, so should be part of config and not decoupled or validated. But maybe it's late to refactor v2 for this

Copy link
Contributor

@HadarIngonyama HadarIngonyama left a comment

Choose a reason for hiding this comment

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

why print and not throw an exception?

// TODO: should throw exception
// TODO: fast twiddles should be used only with mixed radix ntt
// THROW_ICICLE_ERR(IcicleError_t::InvalidArgument, "log_size must be >= 4");
printf("warning: log_size must be at least 4, got %d\n", log_size); // TODO: logging
Copy link
Contributor

Choose a reason for hiding this comment

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

"warning: when using fast twiddles, log_size must be at least 4..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

umm, throwing would be a breaking change

Copy link
Collaborator

@yshekel yshekel left a comment

Choose a reason for hiding this comment

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

I would add the following to init_domain() just before constructing the fast twiddles, to disable it from there rather than calling a function that seems to return success but doesn't do anything.

fast_twiddles_mode &= domain.max_log_size >= 4; // fast twiddles cannot be built for smaller domain

chokobole added a commit to kroma-network/tachyon that referenced this pull request Jul 18, 2024
@vhnatyk
Copy link
Contributor Author

vhnatyk commented Jul 20, 2024

I would add the following to init_domain() just before constructing the fast twiddles, to disable it from there rather than calling a function that seems to return success but doesn't do anything.

fast_twiddles_mode &= domain.max_log_size >= 4; // fast twiddles cannot be built for smaller domain

so this would solve one particular call to a generate_external_twiddles_fast_twiddles_mode , not fix the function itself. And cudaSuccess is default return if no CUDA errors during the call anyway. But since it's called only once perhaps it's indeed ok

@@ -469,6 +469,8 @@ namespace ntt {
CHK_IF_RETURN(mxntt::generate_external_twiddles_generic(
primitive_root, domain.twiddles, domain.internal_twiddles, domain.basic_twiddles, domain.max_log_size,
ctx.stream));

fast_twiddles_mode &= domain.max_log_size >= 4; // fast twiddles cannot be built for smaller domain
Copy link
Contributor

Choose a reason for hiding this comment

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

so the user will think he is using fast twiddles but he isn't? why not just have the exception? or at least add a print in this case

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.

3 participants