jit interpreter branch handling #2481
Conversation
| MachineCallArgumentIdx::Unknown(idx) => { | ||
| let var = &mut vars[*idx] as *mut T; | ||
| LookupCell::Output(unsafe { var.as_mut().unwrap() }) | ||
| let mut block_stack = vec![self.actions.iter()]; |
There was a problem hiding this comment.
handling of the existing actions didn't change, its only code for handling the Branch action that is new
executor/src/witgen/jit/compiler.rs
Outdated
| @@ -29,10 +29,15 @@ use super::{ | |||
| variable::Variable, | |||
| }; | |||
|
|
|||
| pub struct WitgenFunction<T> { | |||
| pub enum WitgenFunction<T: FieldElement> { | |||
There was a problem hiding this comment.
I think it would be better to keep compiler.rs only concerned about compiling effects and transforming parameters to fit the loaded function.
What do you think about putting the enum in function_cache? Then we also don't need a #[cfg(test)]-function for the unit tests here.
| prover_functions, | ||
| ) | ||
| .unwrap(); | ||
| let compiled_jit = !matches!(std::env::var("POWDR_JIT_INTERPRETER"), Ok(val) if val == "1"); |
There was a problem hiding this comment.
As you know, I'm not so happy about changing the behaviour of a function by something that is not an argument to the function.
How do you use this environment variable?
There was a problem hiding this comment.
this seemed like the smallest change to have a way of enabling the interpreter, at least for now:
interpreter is disabled by default, and only enabled when POWDR_JIT_INTERPRETER=1
There was a problem hiding this comment.
You could enable it if the field is not supported by the compiler, for example. Then we test it on a lot of inputs and in the future, we can think about enabling it if the degree is small.
| if condition { | ||
| block_stack.push(if_branch.iter()); | ||
| } else { | ||
| block_stack.push(else_branch.iter()); | ||
| } |
There was a problem hiding this comment.
| if condition { | |
| block_stack.push(if_branch.iter()); | |
| } else { | |
| block_stack.push(else_branch.iter()); | |
| } | |
| block_stack.push(if condition { | |
| if_branch.iter() | |
| } else { | |
| else_branch.iter() | |
| }); |
| #[derive(Debug)] | ||
| enum BranchTest<T: FieldElement> { | ||
| Equal { var: usize, value: T }, | ||
| Less { var: usize, min: T, max: T }, |
There was a problem hiding this comment.
I think in this case, we could maybe call them Inside and Outside.
| if_actions.iter().chain(else_actions).for_each(|a| { | ||
| set.extend(a.writes()); | ||
| }); |
There was a problem hiding this comment.
| if_actions.iter().chain(else_actions).for_each(|a| { | |
| set.extend(a.writes()); | |
| }); | |
| set.extend(if_actions.iter().chain(else_actions).flat_map(|a| a.writes())); |
| use bit_vec::BitVec; | ||
| use itertools::Itertools; | ||
| use powdr_number::GoldilocksField; | ||
|
|
||
| #[test] | ||
| fn branching() { |
| ); | ||
|
|
||
| // 3 - 2 | ||
| params[0] = 3.into(); |
There was a problem hiding this comment.
Maybe avoid some code repetition here?
Something like
let inputs = vec![3,2,0,0,1];
let expected outputs = vec![0];
test(inputs, some_other_args, expected_outputs)?
| @@ -44,8 +45,39 @@ pub struct FunctionCache<'a, T: FieldElement> { | |||
| parts: MachineParts<'a, T>, | |||
| } | |||
|
|
|||
| enum WitgenFunction<T: FieldElement> { | |||
| // TODO We might want to pass arguments as direct function parameters | |||
There was a problem hiding this comment.
Can you move this comment back to CompiledFunction? It mainly concerns the WitgenFunctionParameters, i.e. could also be moved there.
No description provided.