Skip to content

fix: aot traits#2174

Merged
Maillew merged 28 commits into
feat/aot-lightweight-E1-rv32from
feat/aot-traits
Oct 30, 2025
Merged

fix: aot traits#2174
Maillew merged 28 commits into
feat/aot-lightweight-E1-rv32from
feat/aot-traits

Conversation

@Maillew

@Maillew Maillew commented Oct 22, 2025

Copy link
Copy Markdown
Contributor

Add starting code for AotTraits to handle x86 assembly generation.

Refer to design overview here: https://hackmd.io/iSSvk3HsQ0uDqS3i_9UyAA?both#AOT-Traits

@github-actions

This comment has been minimized.

@Maillew Maillew requested a review from jonathanpwang October 22, 2025 22:15
@Maillew Maillew marked this pull request as ready for review October 23, 2025 16:08
Comment thread crates/vm/src/arch/execution.rs Outdated
Ctx: MeteredExecutionCtxTrait;

#[cfg(feature = "aot")]
fn supports_aot_for_opcode(&self, opcode: VmOpcode) -> bool {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good, i can add this

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

@nyunyunyunyu nyunyunyunyu Oct 23, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {}

Comment thread crates/vm/src/arch/execution.rs Outdated
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>;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need pc to jump to a static label?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@nyunyunyunyu nyunyunyunyu Oct 23, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@GunaDD

GunaDD commented Oct 23, 2025

Copy link
Copy Markdown
Contributor

Could you elaborate on how these traits will be integrated with the current create_assembly function in aot.rs?

@nyunyunyunyu

Copy link
Copy Markdown
Contributor

Could you elaborate on how these traits will be integrated with the current create_assembly function in aot.rs?

@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?

@Maillew

Maillew commented Oct 23, 2025

Copy link
Copy Markdown
Contributor Author

Could you elaborate on how these traits will be integrated with the current create_assembly function in aot.rs?

@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

@github-actions

This comment has been minimized.

@github-actions

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 {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confusing. Is this different from a/8?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Return an error instead of panic because this is due to the user inputs.

Comment thread crates/vm/Cargo.toml
# 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"]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove "dep:openvm-rv32im-transpiler"

@github-actions

This comment has been minimized.

GunaDD and others added 2 commits October 28, 2025 21:47
* 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>
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@GunaDD GunaDD changed the base branch from feat/aot to feat/aot-lightweight-E1-rv32 October 30, 2025 19:19
@github-actions

Copy link
Copy Markdown
group app.proof_time_ms app.cycles app.cells_used leaf.proof_time_ms leaf.cycles leaf.cells_used
verify_fibair 238 322,610 2,058,654 - - -
fibonacci 990 1,500,209 2,100,402 - - -
regex 2,373 4,137,502 17,695,216 - - -
ecrecover 702 122,859 2,263,820 - - -
pairing 1,424 1,745,742 25,468,210 - - -

Commit: 26a24b5

Benchmark Workflow

@Maillew Maillew merged commit 47a1db4 into feat/aot-lightweight-E1-rv32 Oct 30, 2025
60 of 64 checks passed
@Maillew Maillew deleted the feat/aot-traits branch October 30, 2025 20:19
Maillew added a commit that referenced this pull request Oct 30, 2025
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-hq

codspeed-hq Bot commented Oct 31, 2025

Copy link
Copy Markdown

CodSpeed Performance Report

Merging #2174 will degrade performances by 65.64%

Comparing feat/aot-traits (26a24b5) with feat/aot-lightweight-E1-rv32 (3181704)

⚠️ Unknown Walltime execution environment detected

Using the Walltime instrument on standard Hosted Runners will lead to inconsistent data.

For the most accurate results, we recommend using CodSpeed Macro Runners: bare-metal machines fine-tuned for performance measurement consistency.

Summary

❌ 11 regressions
✅ 7 untouched
⏩ 42 skipped1

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

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

  1. 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.

nyunyunyunyu pushed a commit that referenced this pull request Oct 31, 2025
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants