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

New Danksharding flow #17

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

New Danksharding flow #17

wants to merge 3 commits into from

Conversation

DmytroTym
Copy link

Danksharding flow based on the new ICICLE API and minor algorithmic improvements

@vhnatyk vhnatyk self-requested a review May 3, 2023 17:16
@vhnatyk vhnatyk assigned vhnatyk and DmytroTym and unassigned vhnatyk May 3, 2023
@@ -8,9 +10,11 @@ pub const FLOW_SIZE: usize = 1 << 12; //4096 //prod flow size
pub const TEST_SIZE_DIV: usize = 1; //TODO: Prod size / test size for speedup
pub const TEST_SIZE: usize = FLOW_SIZE / TEST_SIZE_DIV; //test flow size
pub const M_POINTS: usize = TEST_SIZE;
pub const LOG_M_POINTS: usize = 12;
Copy link
Contributor

Choose a reason for hiding this comment

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

seems it won't work for reduced test vector sizes

pub const SRS_SIZE: usize = M_POINTS;
pub const S_GROUP_SIZE: usize = 2 * M_POINTS;
pub const N_ROWS: usize = 256 / TEST_SIZE_DIV;
pub const LOG_N_ROWS: usize = 8;
Copy link
Contributor

Choose a reason for hiding this comment

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

and here

#[allow(non_upper_case_globals)]
pub fn alternate_flow() {
let D_in_host = get_debug_data_scalar_field_vec("D_in.csv");
let tf_u = &get_debug_data_scalars("roots_u.csv", 1, N_ROWS)[0];
Copy link
Contributor

Choose a reason for hiding this comment

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

pls review for unused vars

let br3_time = Instant::now();

//d0 = MUL_row(d[mu], [S]) 1x8192
let d0: Vec<_> = (0..2 * N_ROWS)
Copy link
Contributor

Choose a reason for hiding this comment

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

actually "batch" version of this loop should be faster, pls look at

multp_vec(&mut d0, &D_b4rbo, 0);
- and
let mut d0 = vec![S; 2 * N_ROWS].concat();
ondevice equivalent should be faster

Copy link
Author

@DmytroTym DmytroTym May 5, 2023

Choose a reason for hiding this comment

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

The third phase was unfinished in that commit, improved now

get_debug_data_points_xy1("P.csv", 2 * N_ROWS, 2 * N_ROWS)
);

assert_ne!(P[12][23], Point::zero()); //dummy check
Copy link
Contributor

Choose a reason for hiding this comment

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

what about icicle #55, probably a quick fix?

#[cfg(test)]
mod tests {
use super::main_flow;
use super::{main_flow, alternate_flow};

#[test]
fn test_main_flow() {
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure current "main_flow" worth keeping if new API is correct and faster

Copy link
Author

@DmytroTym DmytroTym May 5, 2023

Choose a reason for hiding this comment

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

We can remove current main_flow if everyone's happy with the new one, I left it for the sake of easier comparison for now

@DmytroTym DmytroTym marked this pull request as ready for review May 6, 2023 08:00
@DmytroTym
Copy link
Author

Closes #5

{
dim3 dimGrid(nof_rows / TILE_DIM, nof_cols / TILE_DIM, 1);
dim3 dimBlock(TILE_DIM, BLOCK_ROWS, 1);
transpose_kernel <scalar_t> <<<dimGrid, dimBlock>>> (out, in);
Copy link
Contributor

Choose a reason for hiding this comment

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

scalar_t ?

Copy link
Author

Choose a reason for hiding this comment

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

Right, changed to scalar_field_t although why do we have two separate names for the same thing?

@vhnatyk vhnatyk mentioned this pull request May 8, 2023
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.

2 participants