fix: aot traits#2174
Conversation
This comment has been minimized.
This comment has been minimized.
| Ctx: MeteredExecutionCtxTrait; | ||
|
|
||
| #[cfg(feature = "aot")] | ||
| fn supports_aot_for_opcode(&self, opcode: VmOpcode) -> bool { |
There was a problem hiding this comment.
We need assembles for the fallback operations anyway. Can we have a trait(AotFallback) for the fallback operations and generate a default implementation for structs implemented AotFallback? This will give us more flexibility if we want to override the implementation(e.g. a different way to preserve registers).
There was a problem hiding this comment.
sounds good, i can add this
There was a problem hiding this comment.
@nyunyunyunyu I think it may be a better idea to have the default implementations for fallback be feature-gated within the respective Executor and MeteredExecutor traits, instead of as a separate trait, since there shouldn't be a case where a struct has the AotFallback trait, but not the regular Executor trait, since the fallback requires the Executor trait to exist anyways
There was a problem hiding this comment.
My thought is we could have something like this:
pub trait AotFallback: Executor + MeteredExecutor {}
impl<F, E: AotFallback> AotExecutor for E {
fn generate_x86_asm(&self, inst: &Instruction<F>) -> Result<String, AotError> {
...
}
}
When you want a default implementation for an instruction X. You only need to:
impl AotFallback for X {}
| pub trait AotExecutor<F>: Executor<F> { | ||
| /// Generate x86 assembly for the given instruction. Preconditions: Opcode must be supported by | ||
| /// AOT | ||
| fn generate_x86_asm(&self, inst: &Instruction<F>) -> Result<String, AotError>; |
There was a problem hiding this comment.
Do we need pc to jump to a static label?
There was a problem hiding this comment.
we can discuss this with @GunaDD as well, but in the current example, extern_handler returns the updated PC value to register rax, and there is post-processing x86 assembly to determine where to jump next using the jump table. we can continue this convention that it should be on the onus of the generated x86 to write the next PC value to rax, and then it should be able to use any type of label
There was a problem hiding this comment.
I think it means we need to read the jump table based on rax, which brings some extra overheads. If you know pc, for most jumps you can use only 1 instruction to jump into asm_run_pc_base_<pc>(sry the name might be inaccurate) at compile time.
There was a problem hiding this comment.
hm, if thats the case we can pivot so that this function is responsible for handling the updated PC value, and modify the fallback case that uses extern_handler to handle PC changes as well for consistency
There was a problem hiding this comment.
Can you add a comment to show how generated ASMs look like and what the responsibility is? I feel we are not on the same page on that.
I supposed it generates:
balabala
call on_suspend
<exit if suspend>
<instruction execution logic>
<pc/context update>
<jump to next instruction>
@GunaDD mentioned the jump instruction for the dynamic case is:
lea rdx, [rip + map_pc_base]
movsxd rcx, [rdx + r13]
add rcx, rdx
jmp rcx
However if the jump point can be known at AOT compile time, we only need:
jmp asm_execute_pc_<next_pc>
But we need to know the current PC in order to compute next_pc
|
Could you elaborate on how these traits will be integrated with the current |
@Maillew I think we have a working prototype here: https://github.com/openvm-org/openvm/blob/feat/aot-rv32-base-alu/crates/vm/src/arch/aot.rs Maybe let's integrate 1 RV32 instruction as an example? |
Yup, doing this rn |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| let c: i16 = to_i16(inst.c); | ||
| let e: i16 = to_i16(inst.e); | ||
|
|
||
| let xmm_map_reg_a = if (a / 4) % 2 == 0 { |
There was a problem hiding this comment.
I'm confusing. Is this different from a/8?
There was a problem hiding this comment.
yeah that is exactly equal to a/8, I felt like I wanted to be more clear about it but I could probably just do a/8 and write some comments to explain that
| asm_str += &format!(" {} {}, {}\n", asm_opcode, REG_A, c); | ||
| } else { | ||
| // [a:4]_1 <- [a:4]_1 + [c:4]_1 | ||
| assert_eq!(c % 4, 0); |
There was a problem hiding this comment.
Return an error instead of panic because this is due to the user inputs.
| # However tail call elimination is still an incomplete feature in Rust, so the `tco` feature remains experimental until then. | ||
| tco = ["openvm-circuit-derive/tco"] | ||
| # Ahead-of-time execution | ||
| aot = ["dep:libloading"] |
There was a problem hiding this comment.
Remove "dep:openvm-rv32im-transpiler"
This comment has been minimized.
This comment has been minimized.
* Change workflow to enable codpseed bench with aot features * Run the extension tests with aot feature on * Aot skips `test_load_x0` since it is an untrusted program --------- Co-authored-by: Ubuntu <ubuntu@ip-172-31-18-208.ec2.internal>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Commit: 26a24b5 |
Add starting code for `AotTraits` to handle x86 assembly generation. Refer to design overview here: https://hackmd.io/iSSvk3HsQ0uDqS3i_9UyAA?both#AOT-Traits --------- Co-authored-by: Gunadi Gani <87816385+GunaDD@users.noreply.github.com> Co-authored-by: Ubuntu <ubuntu@ip-172-31-18-208.ec2.internal>
CodSpeed Performance ReportMerging #2174 will degrade performances by 65.64%Comparing
|
| Mode | Benchmark | BASE |
HEAD |
Change | |
|---|---|---|---|---|---|
| ❌ | WallTime | benchmark_execute[bubblesort] |
232.8 ms | 333.9 ms | -30.26% |
| ❌ | WallTime | benchmark_execute[fibonacci_iterative] |
279 ms | 400 ms | -30.25% |
| ❌ | WallTime | benchmark_execute[fibonacci_recursive] |
379.2 ms | 553.8 ms | -31.53% |
| ❌ | WallTime | benchmark_execute[keccak256] |
251.4 ms | 354 ms | -28.98% |
| ❌ | WallTime | benchmark_execute[pairing] |
224.8 ms | 276.9 ms | -18.82% |
| ❌ | WallTime | benchmark_execute[quicksort] |
265.1 ms | 373.8 ms | -29.06% |
| ❌ | WallTime | benchmark_execute[revm_snailtracer] |
19.5 ms | 56.6 ms | -65.64% |
| ❌ | WallTime | benchmark_execute[revm_transfer] |
169.3 ms | 292 ms | -42.04% |
| ❌ | WallTime | benchmark_execute[sha256] |
249.7 ms | 356.6 ms | -29.99% |
| ❌ | WallTime | benchmark_execute_metered[bubblesort] |
276.6 ms | 309 ms | -10.49% |
| ❌ | WallTime | benchmark_execute_metered[quicksort] |
314.9 ms | 352.6 ms | -10.7% |
Footnotes
-
42 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
Add starting code for `AotTraits` to handle x86 assembly generation. Refer to design overview here: https://hackmd.io/iSSvk3HsQ0uDqS3i_9UyAA?both#AOT-Traits --------- Co-authored-by: Gunadi Gani <87816385+GunaDD@users.noreply.github.com> Co-authored-by: Ubuntu <ubuntu@ip-172-31-18-208.ec2.internal>
Add starting code for
AotTraitsto handle x86 assembly generation.Refer to design overview here: https://hackmd.io/iSSvk3HsQ0uDqS3i_9UyAA?both#AOT-Traits