Temporary register analysis #3330
Conversation
openvm/src/tmp_register_analysis.rs
Outdated
| let opcode = instruction.opcode.as_usize(); | ||
|
|
||
| if opcode == OPCODE_JALR { | ||
| // For JALR, we don't know the target statically. |
There was a problem hiding this comment.
You can if the previous instruction was AUIPC: https://github.com/powdr-labs/powdr/blob/main/riscv-elf/src/rv64.rs#L211
| } | ||
|
|
||
| if BRANCH_OPCODES_BIGINT.contains(&opcode) || BRANCH_OPCODES.contains(&opcode) { | ||
| // All other branch instructions conditionally add `c` to the current PC, |
| 0 => (vec![], vec![]), | ||
| // PHANTOM | ||
| 1 => { | ||
| // TODO: Not sure what should happen here. |
There was a problem hiding this comment.
https://github.com/powdr-labs/openvm/blob/main/crates/vm/src/arch/segment.rs#L272
Here it doesn't seem to read/write registers. Would need to check other occurrences
| .map(|block| (block.start_pc, register_access_types(block))) | ||
| .collect::<BTreeMap<_, _>>(); | ||
| let block_ids = register_access_types.keys().copied().collect::<Vec<_>>(); | ||
| for i in 0.. { |
There was a problem hiding this comment.
Intuitively it feels like there's a cubic way of doing the entire search, similar to Floyd-Warshall, but I haven't actually tried to prove that
There was a problem hiding this comment.
Don't remember Floyd-Warshall rn, but noting that this runs 13 times in practice on the simple "guest" example. Worst case, this runs as many times as there are basic blocks.
| for (r, succ) in successor_access_types.iter().enumerate() { | ||
| let current = register_access_types.get(block_id).unwrap()[r]; | ||
| if current == RegisterAccessType::Unused && *succ != RegisterAccessType::Unused { | ||
| // Unused becomes whatever the successor is. |
There was a problem hiding this comment.
Is this true?:
For reg r, if in the current block its state is Write, and in the successor it's Read, we shouldn't change it because we can still use that information to potentially prune searches starting at the predecessors of the current block.
There was a problem hiding this comment.
I agree, but that's what the code does, no? It only updates the current block if the state is Unused.
There was a problem hiding this comment.
What we want is:
- Among the successors, read > write > unused
- The current block is only updated with the succor state if the current state is unused
There was a problem hiding this comment.
Although, actually no! I think this is a bug. we should not set the current block to write if any successor is still unknown!
There was a problem hiding this comment.
Haha my first comment "is this true?" was referring to whether my understanding was correcf
There was a problem hiding this comment.
Although, actually no! I think this is a bug. we should not set the current block to write if any successor is still unknown!
Yea agree with this, that's what I was trying to figure out
georgwiese
force-pushed
the
tmp-register-analysis
branch
from
31343bb to
19df4cc
Compare
Following an idea from this doc, checks whether registers (1) written to before being read in a basic block and (2) also written to before read in all possible future basic blocks. If this is the case, the autoprecompile can skip changing their value.
cargo test -r guest_prove_simple -- --nocapture