improve apc interface #3412
improve apc interface #3412
Conversation
| pub bus_interactions: usize, | ||
| } | ||
|
|
||
| impl AirStats { | ||
| pub fn new<F: Clone + Ord + std::fmt::Display>(machine: &SymbolicMachine<F>) -> Self { | ||
| Self { | ||
| main_columns: machine.main_columns().count(), | ||
| constraints: machine.constraints.len(), | ||
| bus_interactions: machine.bus_interactions.len(), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This assumed a single-row arithmetization of the circuit.
| // The cost of adding an apc chip in openvm when it comes to recursion, which we model as its width, since the verifier is linear in the number of columns | ||
| impl Cost<AirMetrics> for OpenVmKnapsackCost { | ||
| fn cost(apc_metrics: AirMetrics) -> usize { | ||
| apc_metrics.total_width() | ||
| } | ||
| } | ||
|
|
||
| impl ApcStats for AirMetrics { | ||
| fn cells_per_call(&self) -> usize { | ||
| self.total_width() * self.rows_per_call | ||
| } | ||
| } |
There was a problem hiding this comment.
This is all we need to know from the client to run cell pgo
| impl<A: Adapter, C: Cost<A>> Candidate<A, C> { | ||
| fn create<Air: ApcArithmetization<A>>( | ||
| apc: Arc<AdapterApc<A>>, | ||
| pgo_program_pc_count: &HashMap<u64, u32>, | ||
| vm_config: AdapterVmConfig<A>, | ||
| max_degree: usize, | ||
| ) -> Self; | ||
| ) -> Self { | ||
| let stats = evaluate_apc::<A, Air>(apc.clone(), vm_config.instruction_handler, max_degree); | ||
|
|
||
| let execution_frequency = | ||
| *pgo_program_pc_count.get(&apc.block.start_pc).unwrap_or(&0) as usize; | ||
|
|
||
| Self { | ||
| apc, | ||
| execution_frequency, | ||
| stats, | ||
| _marker: PhantomData, | ||
| } | ||
| } |
There was a problem hiding this comment.
The input vm_config can technically be replaced by instruction_handler: A::InstructionHandle?
| impl<A: Adapter, C: Cost<A>> KnapsackItem for Candidate<A, C> { | ||
| fn cost(&self) -> usize { | ||
| C::cost(self.stats.after) | ||
| } |
There was a problem hiding this comment.
Wonder why we have Cost::cost in addition to KnapsackItem::cost.
There was a problem hiding this comment.
Basically also why OpenVmKnapsackCost, which implements Cost, in addition to KnapsackItem.
There was a problem hiding this comment.
KnapsackItem is for a generic knapsack algorithm. It is only used internally in the autoprecompile crate.
Cost is the trait implemented by the client, and is linked to the client-specific stats type.
There was a problem hiding this comment.
I see, so this is kind of like making all aspects of the KnapSack algorithm part of APC but just exposing Cost as a client API?
There was a problem hiding this comment.
Yeah you can think of the knapsack stuff as a separate crate, implementation detail inside apc.
Also in theory we could have other algorithms based on the same client cost.
| let stats = evaluate_apc::<A, Air>( | ||
| apc.clone(), | ||
| vm_config.instruction_handler, | ||
| config.degree_bound.identities, |
There was a problem hiding this comment.
Should we use config.degree_bound.bus_interactions instead, because I think deeper inside evaluate_apc it's used in find_interaction_chunks?
Not sure if identities and bus_interactions might really have the same degree though.
There was a problem hiding this comment.
Good question, i'm not sure here. The fact that the tests pass as is suggest this is correct but I'll double check.
qwang98
left a comment
qwang98
left a comment
There was a problem hiding this comment.
Cool PR and one more step towards abstracting APC as much as possible!
Did a thorough review and left a bunch of comments.
The branch name is a leftover from the original exploration of whether we could avoid relying on out stark-backend fork for constraint fetching. It is technically possible but it seemed quite inefficient to filter out logup constraints. It's still interesting to note that we currently do not track the real number of constraints.
Changes in this PR:
CellPgologic into the autoprecompiles crate. TheCandidateis now a struct, not a trait anymore, so that the client only has to implement smaller traits:Cost<A: Adapter>to get the recursion cost from the client-specific air stats andApcStatsto get the number of cells required by call for an apc candidateApcArithmetizationtrait which is used during block selection to find the cost and value of an apc. This part was missing before and we always assumed the SingleRowChip arithmetization was used. This is a bit less relevant now that thePlonkarithmetization got removed, so we may want to remove it from this PR. However we currently have the unwritten assumption that the artihmetization used during block selection matches the one used in the final VM.ApcArithmetizationstill does not force the VM to use the same arithmetization, but it moves in that direction by including the type concretely in PGO.Cellmode: inCellmode they are needed in the selection process, and now in other modes we evaluate the blocks after selection, which reduces the complexity of the types returns by pgo (removing options).