-
Notifications
You must be signed in to change notification settings - Fork 106
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
Logup for o1vm #2757
base: master
Are you sure you want to change the base?
Logup for o1vm #2757
Changes from 25 commits
0a07bba
1ec45d7
1348b91
047534f
91b3ba4
a8a411a
75c6c19
1cb1e50
09e9cbd
a24ef42
70392b7
48ea3d3
d9b472b
55a2441
67730a0
4556330
9f61816
395e322
21524a5
87d4f9c
0b53415
d0236c7
68085b7
5cdbb94
4fc47da
95563f7
d7b1fc9
14dd237
b17b781
2a4b4ac
487da00
681455f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,16 +20,18 @@ use crate::{ | |
registers::Registers, | ||
}, | ||
}, | ||
lookups::Lookup, | ||
lookups::{FixedLookupTables, Lookup, LookupTable, LookupTableIDs}, | ||
preimage_oracle::PreImageOracleT, | ||
utils::memory_size, | ||
}; | ||
use ark_ff::Field; | ||
use core::panic; | ||
use kimchi::o1_utils::Two; | ||
use kimchi_msm::LogupTableID as _; | ||
use log::{debug, info}; | ||
use std::{ | ||
array, | ||
collections::HashMap, | ||
fs::File, | ||
io::{BufWriter, Write}, | ||
}; | ||
|
@@ -64,6 +66,10 @@ impl SyscallEnv { | |
} | ||
} | ||
|
||
// Some type aliases to aid in refactoring to something faster later | ||
type FixedTableMap<ID, F> = HashMap<ID, LookupTable<F>>; | ||
type MultiplicityMap<ID> = HashMap<ID, Vec<u32>>; | ||
|
||
/// This structure represents the environment the virtual machine state will use | ||
/// to transition. This environment will be used by the interpreter. The virtual | ||
/// machine has access to its internal state and some external memory. In | ||
|
@@ -90,6 +96,8 @@ pub struct Env<Fp, PreImageOracle: PreImageOracleT> { | |
pub preimage_key: Option<[u8; 32]>, | ||
pub keccak_env: Option<KeccakEnv<Fp>>, | ||
pub hash_counter: u64, | ||
pub lookup_fixed_tables: FixedTableMap<LookupTableIDs, Fp>, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As discussed, I would recommend using a fixed size array for efficiency, as we know how many, and which, tables are used in the MIPS interpreter. We focus on efficiency for the witness builder. The reason of using a fixed size array is to mostly use the cache of the CPU and to avoid hitting the memory by using pointers. |
||
pub lookup_multiplicities: MultiplicityMap<LookupTableIDs>, | ||
} | ||
|
||
fn fresh_scratch_state<Fp: Field, const N: usize>() -> [Fp; N] { | ||
|
@@ -142,9 +150,18 @@ impl<Fp: Field, PreImageOracle: PreImageOracleT> InterpreterEnv for Env<Fp, PreI | |
} | ||
} | ||
|
||
fn add_lookup(&mut self, _lookup: Lookup<Self::Variable>) { | ||
// No-op, constraints only | ||
// TODO: keep track of multiplicities of fixed tables here as in Keccak? | ||
fn add_lookup(&mut self, lookup: Lookup<Self::Variable>) { | ||
if let Some(idx) = LookupTable::is_in_table(&lookup.table_id, lookup.value) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would recommend using an "assert" as we know exactly which table is used. We don't need this way to do a lookup in a table. |
||
// We found the table, so just add one to the multiplicity. | ||
self.lookup_multiplicities | ||
.get_mut(&lookup.table_id) | ||
.unwrap()[idx] += 1; | ||
} else { | ||
panic!( | ||
"Tried to lookup in non-fixed table: {:?}", | ||
LookupTableIDs::from_u32(lookup.table_id) | ||
); | ||
} | ||
} | ||
|
||
fn instruction_counter(&self) -> Self::Variable { | ||
|
@@ -867,6 +884,22 @@ impl<Fp: Field, PreImageOracle: PreImageOracleT> Env<Fp, PreImageOracle> { | |
} | ||
}; | ||
|
||
let (lookup_fixed_tables, lookup_multiplicities) = { | ||
let mut ft = HashMap::new(); | ||
let mut m = HashMap::new(); | ||
let fixed_tables = LookupTableIDs::all_variants() | ||
.into_iter() | ||
.filter(|x| x.is_fixed()) | ||
.collect::<Vec<_>>(); | ||
|
||
for table_id in fixed_tables { | ||
ft.insert(table_id, table_id.to_fixed_table::<Fp>()); | ||
m.insert(table_id, table_id.to_multiplicities_vec()); | ||
} | ||
|
||
(ft, m) | ||
}; | ||
|
||
Env { | ||
instruction_counter: state.step, | ||
memory: initial_memory.clone(), | ||
|
@@ -891,6 +924,8 @@ impl<Fp: Field, PreImageOracle: PreImageOracleT> Env<Fp, PreImageOracle> { | |
preimage_key: None, | ||
keccak_env: None, | ||
hash_counter: 0, | ||
lookup_fixed_tables, | ||
lookup_multiplicities, | ||
} | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be reverted.