Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

eDSL Kernel Macro Prototype #1121

Closed
wants to merge 1 commit into from
Closed

eDSL Kernel Macro Prototype #1121

wants to merge 1 commit into from

Conversation

TlatoaniHJ
Copy link
Contributor

Discussion

INT-2278

@TlatoaniHJ TlatoaniHJ marked this pull request as draft December 20, 2024 00:20
pub struct CastF;

#[derive(ChipUsageGetter, Chip, InstructionExecutor, From, AnyEnum)]
pub enum CastFExecutor<F: PrimeField32> {
Copy link
Contributor

Choose a reason for hiding this comment

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

structure of this crate is weird: CastF should be in openvm-native-circuit.

This crate should be renamed openvm-native-transpiler

pub mod config;
pub mod deserialize_instruction;

const LONG_FORM_INSTRUCTION_INDICATOR: u32 = (1 << 31) + 115115115;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be some RISC-V instruction with a 6-bit opcode that's one of the designated custom opcodes

pub mod deserialize_instruction;

const LONG_FORM_INSTRUCTION_INDICATOR: u32 = (1 << 31) + 115115115;
const GAP_INDICATOR: u32 = (1 << 31) + 113113113;
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above

Copy link
Contributor

@jonathanpwang jonathanpwang Dec 28, 2024

Choose a reason for hiding this comment

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

this should be moved to extensions/native/guest-macro, I think we can call the crate openvm-native-guest-macro since kernel functions are intended to be called from guest programs

edition = \"2021\"

[dependencies]
openvm-native-compiler = { path = \"../../../../extensions/native/compiler\" }
Copy link
Contributor

Choose a reason for hiding this comment

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

we need a way to have dependency versions consistent with the workspace without hardcoding

Copy link
Contributor

Choose a reason for hiding this comment

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

harcoding path is also not good

type F = BabyBear;
type EF = BinomialExtensionField<BabyBear, 4>;

<===>
Copy link
Contributor

Choose a reason for hiding this comment

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

use format! to include the OUTSIDE_PLACEHOLDER by constant name

const IMMEDIATE_ADDRESS_SPACE: usize = 0;
const RUST_REGISTER_ADDRESS_SPACE: usize = 1;
const KERNEL_ADDRESS_SPACE: usize = 5;
const LONG_FORM_INSTRUCTION_INDICATOR: u32 = (1 << 31) + 115115115;
Copy link
Contributor

Choose a reason for hiding this comment

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

these should be defined in a common crate that can be imported by both this crate and the transpiler crate

mod transportation;

const IMMEDIATE_ADDRESS_SPACE: usize = 0;
const RUST_REGISTER_ADDRESS_SPACE: usize = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

we already have this constant in openvm-instructions


const IMMEDIATE_ADDRESS_SPACE: usize = 0;
const RUST_REGISTER_ADDRESS_SPACE: usize = 1;
const KERNEL_ADDRESS_SPACE: usize = 5;
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be moved to openvm-instructions

Copy link
Contributor

Choose a reason for hiding this comment

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

General thoughts:

  • This is actually a bit messy and potentially shows the benefits of having a DSL instead of eDSL (then you can just take in a text file and compile it)
  • For now I think we should not include the generation of the serialized openvm assembly file in the proc-macro, and just let the macro load it from some pre-existing file

const REGISTER_LIMB_SIZE: usize = 256;

#[proc_macro]
pub fn edsl_kernel(input: proc_macro::TokenStream) -> proc_macro::TokenStream {
Copy link
Contributor

Choose a reason for hiding this comment

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

native_kernel is a better name

result
}

fn u32_to_directive(x: u32) -> String {
Copy link
Contributor

Choose a reason for hiding this comment

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

unnecessary, .insn supports a direct 32-bit version

Copy link
Contributor

Choose a reason for hiding this comment

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

directioinally correct, but we should use quote! and paste! whenever possible. only the raw asm! block should be generated using string manipulation

@@ -10,5 +10,33 @@ pub trait TranspilerExtension<F> {
/// the next contiguous section of RISC-V instructions into an [`Instruction`].
/// It returns `None` if it cannot transpile. Otherwise it returns `(instruction, how_many_u32s)` to indicate that
/// `instruction_stream[..how_many_u32s]` should be transpiled into `instruction`.
Copy link
Contributor

Choose a reason for hiding this comment

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

update comment

@@ -10,5 +10,33 @@ pub trait TranspilerExtension<F> {
/// the next contiguous section of RISC-V instructions into an [`Instruction`].
/// It returns `None` if it cannot transpile. Otherwise it returns `(instruction, how_many_u32s)` to indicate that
/// `instruction_stream[..how_many_u32s]` should be transpiled into `instruction`.
fn process_custom(&self, instruction_stream: &[u32]) -> Option<(Instruction<F>, usize)>;
fn process_custom(&self, instruction_stream: &[u32]) -> Option<TranspilerOutput<F>>;
Copy link
Contributor

Choose a reason for hiding this comment

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

noting the return type is a breaking change, but is necessary because we want one Option<Instruction<F>> per 32-bit chunk

Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer if we just had a file with 32-bits per line, and then the macro loaded that file and converted into asm! block

}

impl<F> TranspilerOutput<F> {
pub fn one_to_one(instruction: Instruction<F>) -> Self {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we could implement From<Instruction<F>> for TranspilerOutput<F>

),
1,
));
return Some(TranspilerOutput::one_to_one(Instruction::phantom(
Copy link
Contributor

Choose a reason for hiding this comment

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

see my note above about using into()

Copy link
Contributor

@jonathanpwang jonathanpwang left a comment

Choose a reason for hiding this comment

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

I think the general mechanisms are in place, but a lot of refactoring / adjustments to be made.

The compilation of eDSL within proc-macro seems like it will have versioning issues, and I'm also concerned it is too slow for rust-analyzer. I think for this first version, we should focus on the case where you just compile the eDSL separately into serialized 32-bit instruction format, and then the kernel proc-macro is focused on inserting those serialized instructions into the ELF.

@jonathanpwang
Copy link
Contributor

replaced by #1191

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.

2 participants