make the RISCV executor look at the optimized PIL for link information #2232
Conversation
This reverts commit d31f815.
pacheco
changed the title
| let end = s.find(']').unwrap(); | ||
| s[start..end].parse::<u8>().unwrap() | ||
| }) | ||
| .unwrap(); |
There was a problem hiding this comment.
This seems kinda costly if it has to run every time? Wouldn't it be better to have a map in the caller and just pass the number like before?
There was a problem hiding this comment.
yes, this is ugly, i'm working on a separate PR for these details/performance (this here should be cached)
| @@ -4,17 +4,20 @@ use powdr_number::{FieldElement, LargeInt}; | |||
|
|
|||
| use std::collections::HashMap; | |||
|
|
|||
| fn only_column_name(name: &str) -> &str { | |||
| name.rfind("::").map(|i| &name[i + 2..]).unwrap_or(name) | |||
There was a problem hiding this comment.
maybe leave a comment on why i + 2?
| /// Which of the witness columns are selectors, if any | ||
| const SELECTORS: &'static str; | ||
| /// Row block size | ||
| const BLOCK_SIZE: u32; | ||
| /// Add an operation to the submachine trace | ||
| fn add_operation<F: FieldElement>( | ||
| trace: &mut SubmachineTrace<F>, | ||
| // pil lookup RHS values (including selector idx, if present) | ||
| selector_idx: Option<u8>, | ||
| selector: Option<&str>, |
There was a problem hiding this comment.
same question here re number vs string, string sounds costly?
There was a problem hiding this comment.
here the string is actually better, we just use the string as a key (before we had to actually construct the string here)
| pub fn links_from_pil<F: FieldElement>(pil: &Analyzed<F>) -> Vec<Identity<F>> { | ||
| pil.identities | ||
| .iter() | ||
| .filter(|id| matches!(id, Identity::Permutation(_) | Identity::Lookup(_))) |
There was a problem hiding this comment.
I guess later we'll also need to support the Phantom versions, and/or bus sends/receives. It might actually be simpler at that point.
There was a problem hiding this comment.
yeah, I didn't really look into it here, but should not be to bad I imagine
riscv-executor/src/pil.rs
Outdated
| } | ||
| } | ||
| } | ||
| panic!("identity {id} doesnt exist") |
There was a problem hiding this comment.
| panic!("identity {id} doesnt exist") | |
| panic!("identity {id} doesn't exist") |
| } | ||
| if let Identity::Lookup(lookup) = l { | ||
| if lookup.id == id { | ||
| return None; |
There was a problem hiding this comment.
Why do lookups return None?
There was a problem hiding this comment.
only Permutations have a selector
| // main_keccakf, | ||
| // main_arith, | ||
| /// Enum with asm machine RISCV instruction. Helps avoid using raw strings in the code. | ||
| macro_rules! instructions { |
There was a problem hiding this comment.
Can't we somehow get the same info from the asm file?
There was a problem hiding this comment.
it won't be an enum then...
| loop { | ||
| let stm = statements[curr_pc as usize]; | ||
|
|
||
| log::trace!("l {curr_pc}: {stm}",); | ||
|
|
||
| e.step += 4; | ||
|
|
||
| count += 1; | ||
| if count % 10000 == 0 { |
There was a problem hiding this comment.
I think we should make this number larger otherwise it might be too many messages for large programs. Maybe 100k?
There was a problem hiding this comment.
the message is printed every second, this is just how frequently we sample the clock
columns should still be present but empty
riscv-executor/src/lib.rs
Outdated
| .par_bridge() | ||
| .flat_map(|(m, mut machine, ops)| { | ||
| // apply the operation to the submachine | ||
| println!("machine: {:?}", m.name()); |
This PR modifies the RISCV executor to look into the optimized PIL for lookup/permutation definitions.
In particular, to figure out which selectors are assigned for each link, but also with the intent of later integration with witgen (which needs the identity ids for "submachine calls")