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

Batched ec additions msm #615

Merged
merged 7 commits into from
Oct 10, 2024
Merged

Batched ec additions msm #615

merged 7 commits into from
Oct 10, 2024

Conversation

Koren-Brand
Copy link
Contributor

Describe the changes

Batched EC addition was added for MSM, improving performance. Also previous bug where c divides scalar bit width is fixed.

Linked Issues

Resolves #

Signed-off-by: Koren-Brand <koren@ingonyama.com>
Signed-off-by: Koren-Brand <koren@ingonyama.com>
@@ -9,6 +9,8 @@
#define MANAGER_SLEEP_USEC 10
#define THREAD_SLEEP_USEC 1

// #define LOG_UTILIZATION
Copy link
Collaborator

Choose a reason for hiding this comment

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

do you plan to keep it here?

@@ -14,10 +14,16 @@
#include "icicle/msm.h"
#include "tasks_manager.h"
#include "icicle/backend/msm_config.h"
#ifdef LOG_UTILIZATION
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you want the code to have this flow for logging? it's kind of ugly in the code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can include timer either way if it's important


using namespace icicle;
using namespace curve_config;

#define LOG_EC_BATCH_SIZE 2
Copy link
Collaborator

Choose a reason for hiding this comment

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

is 2 the optimum?

Copy link
Collaborator

Choose a reason for hiding this comment

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

also the name is not that clear

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was chosen as preparation for AVX but I can run a few checks to see what works better

@@ -352,20 +421,23 @@ void Msm<A, P>::phase1_bucket_accumulator(const scalar_t* scalars, const A* base
// Handle required preprocess of base P
A base =
m_are_points_mont ? A::from_montgomery(bases[m_precompute_factor * i + j]) : bases[m_precompute_factor * i + j];
// TODO move to preprocess before precompute to avoid repeating conversions
Copy link
Collaborator

Choose a reason for hiding this comment

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

todo when? also for the one below

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 not this week, I forgot to remove TODOs before the PR.

@@ -6,7 +6,7 @@
#include <cassert>
#include "timer.hpp"

// #define DUMMY_TYPES
#define DUMMY_TYPES
Copy link
Collaborator

Choose a reason for hiding this comment

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

didn't you want to remove this test? I don't know if you should but you wanted I think

const int N = 1 << logn;
const int precompute_factor = 2;
const int precompute_factor = 8;
Copy link
Contributor

Choose a reason for hiding this comment

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

how about random

(int)std::floor(std::log2((double)(nof_threads * TASKS_PER_THREAD - 1) / (double)(2 * m_num_bms))), 0)),
m_num_bm_segments(std::min((int)(1 << m_log_num_segments), (int)(m_num_bms * m_num_bkts))),
(int)std::floor(
std::log2((double)(nof_threads * TASKS_PER_THREAD * EC_BATCH_SIZE - 1) / (double)(2 * m_num_bms))),
Copy link
Contributor

Choose a reason for hiding this comment

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

do you really need the double casting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can remove one of the (double)s if that is what you mean.

Signed-off-by: Koren-Brand <koren@ingonyama.com>
@Koren-Brand Koren-Brand merged commit fdd9fca into main Oct 10, 2024
28 checks passed
@Koren-Brand Koren-Brand deleted the batched_ec_additions_msm branch October 10, 2024 16:17
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