Unify single step and block processor. #2317
Conversation
chriseth
force-pushed
the
unified_processor
branch
from
6c48318 to
1340f79
Compare
chriseth
force-pushed
the
unified_processor
branch
from
1340f79 to
fbf5236
Compare
|
All tests except two are missing: The first one is the "not_stackable", which is just not yet implemented. |
| @@ -77,8 +77,9 @@ pub fn compile_effects<T: FieldElement>( | |||
|
|
|||
| record_start("JIT-compilation"); | |||
| let start = std::time::Instant::now(); | |||
| log::trace!("Calling cargo..."); | |||
| let r = powdr_jit_compiler::call_cargo(&code); | |||
| let opt_level = 0; | |||
There was a problem hiding this comment.
So now we can no longer measure the runtime speed of optimized binary?
There was a problem hiding this comment.
Do you do that regularly? If you want to, you can change this number here ;)
The thing is that cargo nextest run should just work without the need for a complicated setup where we tune environment variables.
There was a problem hiding this comment.
OK, fine with me now. I think it should be a CLI argument, but we can do that later. In the tests we'll always want to use 0 as long as we don't share compilation between tests.
| if complete_rows.len() >= self.block_size { | ||
| let (min, max) = complete_rows.iter().minmax().into_option().unwrap(); | ||
| // TODO instead of checking for consequitive rows, we could also check | ||
| // that they "fit" the next block. |
There was a problem hiding this comment.
Are you saying that there could be gaps? Why? Eventually, we want the machine calls to be handled on all rows of the trace, right? The call being "complete" does not mean that it actually executed, it could be that the selector was 0, in which case it is also considered complete.
There was a problem hiding this comment.
But if the selector is zero, it will also be zero in the other, overlapping block. What I'm saying is that we can have
A
B B B B B
C C C C C
D D D D
...
i.e. there is a gap of one that is filled by the previous / next block.
We should verify that with real machines, but I think the number of complete rows should be exactly block_size and row i should be complete if and only if row i + block_size is not complete.
There was a problem hiding this comment.
OK, sure ^^ Seems very esoteric though.
| let block_size = 1; | ||
| let witgen = WitgenInference::new(self.fixed_data, NoEval, known_variables); | ||
|
|
||
| let max_branch_depth = 6; |
There was a problem hiding this comment.
Again, please turn into a constant. Does it need to be a variable to Processor::new? Would we pass a different value in different situations? Otherwise, the constant could be in the processor file.
There was a problem hiding this comment.
We could pass different values for different machines. For example, a VM could find the pc lookup and use number of instruction variables * 2 or something.
executor/src/witgen/jit/processor.rs
Outdated
|
|
||
| /// After solving, the known cells should be such that we can stack different blocks. | ||
| /// TODO the same is actually true for machine calls. | ||
| fn check_block_shape( |
There was a problem hiding this comment.
Why put this here, if it is not run when called from the single step processor?
There was a problem hiding this comment.
Because the block_machine_processor does not have access to witgen any more
There was a problem hiding this comment.
Also it needs to be run in each branch.
There was a problem hiding this comment.
Right. But that means that the machine could have a different shape in each branch!
I think the proper way to handle this would be to let the processor do it's thing and then look at the referenced cells of the generated effects.
Co-authored-by: Georg Wiese <[email protected]>
… into unified_processor
Extracts the common code in single step processor and block processor into a unified "processor".