Skip to content
This repository has been archived by the owner on Nov 4, 2024. It is now read-only.

Make MaxWidthCellManager work with fixed signals - Issue #97 #135

Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -24,4 +24,4 @@ serde_json = "1.0"
[patch."https://github.com/privacy-scaling-explorations/halo2.git"]
halo2_proofs = { git = "https://github.com/appliedzkp/halo2.git", rev = "d3746109d7d38be53afc8ddae8fdfaf1f02ad1d7", features = [
"circuit-params",
] }
] }
13 changes: 5 additions & 8 deletions examples/poseidon.rs
Original file line number Diff line number Diff line change
Expand Up @@ -661,25 +661,22 @@ fn poseidon_super_circuit<F: PrimeField + Eq + Hash>(
lens: Lens,
) -> SuperCircuit<F, ValuesAndLens<F>> {
super_circuit::<F, ValuesAndLens<F>, _>("poseidon", |ctx| {
let single_config = config(SingleRowCellManager {}, SimpleStepSelectorBuilder {});
let config = config(MaxWidthCellManager::new(2, 2, true), SimpleStepSelectorBuilder {});
Copy link
Collaborator

Choose a reason for hiding this comment

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

@atsupontio I think it would be better if run the example with both Cell Managers

Copy link
Author

Choose a reason for hiding this comment

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

@leolara Thank you!
I'll fix it!

let (_, constants_table) = ctx.sub_circuit(
single_config.clone(),
config.clone(),
poseidon_constants_table,
lens.n_inputs + 1,
);
let (_, matrix_table) =
ctx.sub_circuit(single_config, poseidon_matrix_table, lens.n_inputs + 1);
ctx.sub_circuit(config.clone(), poseidon_matrix_table, lens.n_inputs + 1);

let params = CircuitParams {
lens,
constants_table,
matrix_table,
};
let maxwidth_config = config(
MaxWidthCellManager::new(2, true),
SimpleStepSelectorBuilder {},
);
let (poseidon, _) = ctx.sub_circuit(maxwidth_config, poseidon_circuit, params);

let (poseidon, _) = ctx.sub_circuit(config, poseidon_circuit, params);

ctx.mapping(move |ctx, values| {
ctx.map(&poseidon, values);
Expand Down
64 changes: 55 additions & 9 deletions src/plonkish/compiler/cell_manager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -231,24 +231,26 @@ impl CellManager for SingleRowCellManager {

#[derive(Debug, Default, Clone)]
pub struct MaxWidthCellManager {
max_width: usize,
max_width_advice: usize,
max_width_fixed: usize,
same_height: bool,
}

impl MaxWidthCellManager {
pub fn new(max_width: usize, same_height: bool) -> Self {
pub fn new(max_width_advice: usize, max_width_fixed: usize, same_height: bool) -> Self {
Self {
max_width,
max_width_advice,
max_width_fixed,
same_height,
}
}
}

impl CellManager for MaxWidthCellManager {
fn place<F>(&self, unit: &mut CompilationUnit<F>) {
if (!unit.shared_signals.is_empty() || !unit.fixed_signals.is_empty()) && !self.same_height
if (!unit.shared_signals.is_empty()) && !self.same_height
Copy link
Collaborator

Choose a reason for hiding this comment

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

@atsupontio this must be kept. Fixed signals should not work when same_height is disabled

{
panic!("Shared signals and fixed signals are not supported for MaxWidthCellManager, which might return steps with variable heights.");
panic!("Shared signals are not supported for MaxWidthCellManager, which might return steps with variable heights.");
}

let mut placement = Placement {
Expand Down Expand Up @@ -287,7 +289,7 @@ impl CellManager for MaxWidthCellManager {
);

forward_signal_column += 1;
if forward_signal_column >= self.max_width {
if forward_signal_column >= self.max_width_advice {
forward_signal_column = 0;
forward_signal_row += 1;
}
Expand All @@ -299,6 +301,48 @@ impl CellManager for MaxWidthCellManager {
forward_signal_row
} as u32;



let mut fixed_signal_column: usize = 0;
let mut fixed_signal_row: usize = 0;

for fixed_signal in unit.fixed_signals.iter() {
let column = if placement.columns.len() <= fixed_signal_column {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@atsupontio you should add some tests, this will not work. Because placement.columns contains both advice and fixed signals, if there are both, for example when fixed_signal_column == 0 but there are already columns that are advice the fixed column will not be created and the wrong column will be picked up

let column = if let Some(annotation) = unit.annotations.get(&fixed_signal.uuid())
{
Column::fixed(format!("mwcm fixed signal {}", annotation))
} else {
Column::fixed("mwcm fixed signal")
};

placement.columns.push(column.clone());
column
} else {
placement.columns[fixed_signal_column].clone()
};

placement.fixed.insert(
*fixed_signal,
SignalPlacement {
column,
rotation: fixed_signal_row as i32,
},
);

fixed_signal_column += 1;
if fixed_signal_column >= self.max_width_fixed {
fixed_signal_column = 0;
fixed_signal_row += 1;
}
}

placement.base_height = if fixed_signal_column != 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@atsupontio what if there is more rows in the advice than in the fixed?

fixed_signal_row + 1
} else {
fixed_signal_row
} as u32;


for step in unit.step_types.values() {
let mut step_placement = StepPlacement {
height: if forward_signal_column > 0 {
Expand Down Expand Up @@ -337,7 +381,7 @@ impl CellManager for MaxWidthCellManager {
step_placement.height = (internal_signal_row + 1) as u32;

internal_signal_column += 1;
if internal_signal_column >= self.max_width {
if internal_signal_column >= self.max_width_advice {
internal_signal_column = 0;
internal_signal_row += 1;
}
Expand Down Expand Up @@ -402,7 +446,8 @@ mod tests {
// forward signals: a, b; step1 internal: c1, d, e; step2 internal c2

let cm = MaxWidthCellManager {
max_width: 2,
max_width_advice: 2,
max_width_fixed: 2,
same_height: false,
};

Expand Down Expand Up @@ -472,7 +517,8 @@ mod tests {
// forward signals: a, b; step1 internal: c1, d, e; step2 internal c2

let cm = MaxWidthCellManager {
max_width: 2,
max_width_advice: 2,
max_width_fixed: 2,
same_height: true,
};

Expand Down