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.

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

ntt cpu #533

Merged
merged 1 commit into from
Jul 15, 2024
Merged

ntt cpu #533

merged 1 commit into from
Jul 15, 2024

Conversation

ShanieWinitz
Copy link
Contributor

No description provided.

@ShanieWinitz ShanieWinitz requested a review from yshekel June 5, 2024 11:14
@ShanieWinitz ShanieWinitz marked this pull request as ready for review June 5, 2024 11:14
@ShanieWinitz ShanieWinitz marked this pull request as draft June 5, 2024 11:15
@ShanieWinitz ShanieWinitz marked this pull request as ready for review June 5, 2024 11:38
@yshekel yshekel force-pushed the yshekel/V3 branch 5 times, most recently from e7add84 to ba7d500 Compare June 10, 2024 08:48
@ShanieWinitz ShanieWinitz force-pushed the swinitz/cpuNtt branch 2 times, most recently from 847ca3f to eb79777 Compare June 17, 2024 14:30
@yshekel yshekel force-pushed the yshekel/V3 branch 2 times, most recently from 600f4e2 to 707c474 Compare June 17, 2024 15:48
@ShanieWinitz ShanieWinitz force-pushed the swinitz/cpuNtt branch 7 times, most recently from 82a9534 to fd29c06 Compare June 18, 2024 12:44
yshekel
yshekel reviewed Jun 24, 2024
View reviewed changes
icicle_v3/tests/test_field_api.cpp Outdated
auto out_main = std::make_unique<TypeParam[]>(N);
auto out_ref = std::make_unique<TypeParam[]>(N);
// // Randomize config
// TODO - iterate over different configs
Copy link
Collaborator

Choose a reason for hiding this comment

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

when are you going to do it? I think it's better than random since we know what we check. Otherwise CI can pass sometimes and fail in others, based on the randomized case.

Copy link
Collaborator

Choose a reason for hiding this comment

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

update: since rust tests are covering many cases, this test can doesn't have to as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

yshekel
yshekel reviewed Jun 24, 2024
View reviewed changes
icicle_v3/tests/test_field_api.cpp Outdated
const NTTDir dir = NTTDir::kForward;

// Print config
ICICLE_LOG_DEBUG << "logn: " << logn;
Copy link
Collaborator

Choose a reason for hiding this comment

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

when done, please remove those logs.

yshekel
yshekel reviewed Jun 24, 2024
View reviewed changes
icicle_v3/tests/test_field_api.cpp Outdated
config.batch_size = batch_size; // default: 1
config.columns_batch = columns_batch; // default: false
config.ordering = ordering; // default: kNN
config.are_inputs_on_device = true; // TODO, ask yuval why set to true?
Copy link
Collaborator

Choose a reason for hiding this comment

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

for CPU it doesn't matter but for devices with own memory, like GPU/ZPU/FPGA, we need to know where the data resides.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

yshekel
yshekel reviewed Jun 24, 2024
View reviewed changes
icicle_v3/backend/cpu/src/field/cpu_ntt.cpp
REGISTER_NTT_INIT_DOMAIN_BACKEND("CPU", (cpu_ntt_init_domain));
REGISTER_NTT_INIT_DOMAIN_BACKEND("CPU_REF", (cpu_ntt_init_domain));
Copy link
Collaborator

Choose a reason for hiding this comment

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

do CPU and CPU_REF share the domain or will it be allocated for each?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As we talked, I left it like this.

yshekel
yshekel reviewed Jun 24, 2024
View reviewed changes
icicle_v3/backend/cpu/src/field/cpu_ntt.cpp

#ifdef EXT_FIELD
REGISTER_NTT_EXT_FIELD_BACKEND("CPU", (cpu_ntt<scalar_t, extension_t>));
Copy link
Collaborator

Choose a reason for hiding this comment

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

probably this case also needs to be registered for CPU_REF.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

yshekel
yshekel reviewed Jun 24, 2024
View reviewed changes
icicle_v3/backend/cpu/include/ntt_template.h Outdated

using namespace field_config;
using namespace icicle;
namespace ntt_template {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would also rename the namespace to maybe cpu_ntt

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe "cpu_ntt_h" or something else? "cpu_ntt" creates errors in cpu_ntt.cpp

yshekel
yshekel reviewed Jul 7, 2024
View reviewed changes
icicle_v3/backend/cpu/include/cpu_ntt.h Outdated
// calculate twiddles
// Note: radix-2 INTT needs ONE in last element (in addition to first element), therefore have n+1 elements

std::unique_ptr<S[]> temp_twiddles(new S[s_ntt_domain.max_size + 1]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

please add comment why you need the temp twiddles. It's because otherwise twiddles are not nullptr but still not computed right?

BTW you'd better use std::make_unique<S[]>(max_size+1) instead of new.

@ShanieWinitz ShanieWinitz force-pushed the swinitz/cpuNtt branch 2 times, most recently from 1979bb0 to 5cc8257 Compare July 7, 2024 14:22
yshekel
yshekel reviewed Jul 7, 2024
View reviewed changes
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.

overall looks good but I wrote a few comments

icicle_v3/backend/cpu/include/cpu_ntt.h Show resolved Hide resolved
icicle_v3/backend/cpu/include/cpu_ntt.h Outdated
NTTDir dir = NTTDir::kForward,
bool columns_batch = false)
{
int size = 1 << logn;
Copy link
Collaborator

Choose a reason for hiding this comment

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

note that this can potentially overflow

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why? isn't logn maximum 30?

icicle_v3/backend/cpu/include/cpu_ntt.h Outdated
template <typename S = scalar_t, typename E = scalar_t>
eIcicleError cpu_ntt_ref(const Device& device, const E* input, int size, NTTDir dir, NTTConfig<S>& config, E* output)
{
if (size & (size - 1)) { return eIcicleError::INVALID_ARGUMENT; }
Copy link
Collaborator

Choose a reason for hiding this comment

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

use ICICLE_LOG_ERROR to log errors to user in case of invalid inputs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

icicle_v3/backend/cpu/include/cpu_ntt.h Show resolved Hide resolved
icicle_v3/backend/cpu/include/cpu_ntt.h Show resolved Hide resolved
icicle_v3/backend/cpu/include/cpu_ntt.h Outdated
temp_cosets[i] = coset;
}
arbitrary_coset = temp_cosets.get();
temp_cosets.release();
Copy link
Collaborator

Choose a reason for hiding this comment

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

release?
Why don't you allocate the arbitrary coset directly? as a unique_ptr I mean

icicle_v3/backend/cpu/include/cpu_ntt.h Outdated
break;
}
}
if (coset_stride == 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

please add comments explaining what is being done in every step. It would make the code easier to understand

icicle_v3/backend/cpu/include/cpu_ntt.h Outdated

// NTT/INTT
if (dit) {
ICICLE_LOG_DEBUG << "DIT";
Copy link
Collaborator

Choose a reason for hiding this comment

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

either print a clear message or remove

icicle_v3/backend/cpu/include/cpu_ntt.h Outdated
if (dir == NTTDir::kInverse) {
// Normalize results
S inv_size = S::inv_log_size(logn);
ICICLE_LOG_DEBUG << "inv_size: " << inv_size;
Copy link
Collaborator

Choose a reason for hiding this comment

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

here too

@ShanieWinitz ShanieWinitz force-pushed the swinitz/cpuNtt branch 3 times, most recently from dba7352 to 2e73477 Compare July 7, 2024 15:59
@yshekel yshekel force-pushed the yshekel/V3 branch 2 times, most recently from b351426 to 4473fc3 Compare July 8, 2024 10:39
@ShanieWinitz ShanieWinitz force-pushed the swinitz/cpuNtt branch 8 times, most recently from 9251bd1 to fb4e977 Compare July 8, 2024 15:39
@yshekel yshekel merged commit 956b982 into yshekel/V3 Jul 15, 2024
13 checks passed
@yshekel yshekel deleted the swinitz/cpuNtt branch July 15, 2024 08:27
yshekel pushed a commit that referenced this pull request Jul 17, 2024
@ShanieWinitz @yshekel
NTT simple cpu backend (#533)
36c8799
@ShanieWinitz ShanieWinitz restored the swinitz/cpuNtt branch August 21, 2024 13:56
@ShanieWinitz ShanieWinitz deleted the swinitz/cpuNtt branch August 21, 2024 13:57
@ShanieWinitz ShanieWinitz restored the swinitz/cpuNtt branch August 21, 2024 13:58
@ShanieWinitz ShanieWinitz deleted the swinitz/cpuNtt branch August 21, 2024 14:01
@ShanieWinitz ShanieWinitz restored the swinitz/cpuNtt branch August 21, 2024 14:11
@ShanieWinitz ShanieWinitz deleted the swinitz/cpuNtt branch August 21, 2024 14:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yshekel yshekel yshekel 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.
2 participants
@ShanieWinitz @yshekel