compiler options and codegen split #1879

leonardoalt · 2024-10-08T12:19:08Z

Extracted from #1790.

This PR:

creates the new CompilerOptions that encapsulate precompiles and common options, needed by both cli-rs and tests
splits codegen, runtime and bootloader into an outer facing dispatcher based on the compiler options, and internal versions that currently are just copies of the previous implementation that is specific to Goldilocks and larger modulus.

After this PR, the riscv-bb PR will simply add analogous 17-bit codegen, runtime and bootloader files.

leonardoalt · 2024-10-08T12:21:43Z

riscv/src/code_gen64.rs

+use crate::CompilerOptions;
+
+/// Translates a RISC-V program to POWDR ASM
+/// with constraints that work for a field >= the Goldilocks modulus.


This file contains everything that was "removed" according to GH from the file above code_gen.rs, and I changed the text in this line. Everything else is untouched.

leonardoalt · 2024-10-08T12:23:04Z

riscv/src/continuations/bootloader64.rs

+    MERKLE_TREE_DEPTH, NUM_PAGES_INDEX, N_LEAVES_LOG, PAGE_INPUTS_OFFSET, PAGE_NUMBER_MASK,
+    PAGE_SIZE_BYTES, PC_INDEX, REGISTER_NAMES, SHUTDOWN_START, WORDS_PER_HASH, WORDS_PER_PAGE,
+};
+


This "new" file contains everything "removed" from file above bootloader.rs that is specific to the 64 bits version of the constraints, unmodified.

leonardoalt · 2024-10-08T12:24:24Z

riscv/src/runtime64.rs

+use itertools::Itertools;
+
+use crate::code_gen::Register;
+


This file contains everything that was "removed" from file above runtime.rs, with all the 64-bit specific stuff, unmodified.

leonardoalt · 2024-10-08T12:25:00Z

riscv/src/runtime64.rs

+            runtime = runtime.with_arith();
+        }
+        runtime
+    }


This type, its constructor and its usage are the only changes.

leonardoalt · 2024-10-08T12:25:40Z

riscv/src/runtime.rs

+#[derive(Clone)]
+pub struct SyscallImpl(pub Vec<FunctionStatement>);
+
+pub trait Runtime {


This trait could even go away potentially, since we don't use it as dyn or Box anymore.

leonardoalt · 2024-10-08T12:25:57Z

riscv/src/runtime.rs

    /// Full path to machine (e.g, `path::to::Machine`)
-    path: SymbolPath,
+    pub path: SymbolPath,


Note to self: these pubs can probably go now too.

leonardoalt · 2024-10-08T12:26:22Z

riscv/tests/common/mod.rs

 use std::{
    path::{Path, PathBuf},
    process::Command,
 };

 /// Like compiler::test_util::run_pilcom_asm_string, but also runs RISCV executor.
-pub fn verify_riscv_asm_string<S: serde::Serialize + Send + Sync + 'static>(
+pub fn verify_riscv_asm_string<T: FieldElement, S: serde::Serialize + Send + Sync + 'static>(


Unfortunately we do need T around here because of Pipeline and inputs.

pacheco · 2024-10-09T18:44:29Z

cli-rs/src/main.rs

+    let libs = coprocessors_to_options(coprocessors)?;
+    let options = CompilerOptions::new(field, libs, continuations);
+    powdr_riscv::compile_riscv_elf(input_file, Path::new(input_file), options, output_dir, true)
+        .ok_or_else(|| vec!["could not translate RISC-V executable".to_string()])?;

    Ok(())
 }

 #[allow(clippy::too_many_arguments)]
 fn execute<F: FieldElement>(


execute already takes F, it doesn't need the field: KnownField parameter

yea I kept both because they are used in different ways, so didn't want to mix them up. Fine with removing, but want to double check after explaining why. Should I still remove it?

yes, i'd call F::known_field inside

there's a similar situation in another part of the code, but there I think F can be removed, because there you match on the known field to then pick a concrete field

pacheco · 2024-10-09T19:00:57Z

pipeline/src/test_util.rs

-    let mut pipeline_bb = make_prepared_pipeline(file_name, inputs_bb, vec![]);
-    pipeline_bb.compute_witness().unwrap();
+    let pipeline_bb = make_prepared_pipeline(file_name, inputs_bb, vec![]);
+    test_plonky3_pipeline(pipeline_bb);


should this go into this PR? not that I see any particular issue

I can remove, it just came with the same patch when I extracted

pacheco · 2024-10-09T19:05:50Z

pipeline/tests/asm.rs

@@ -24,7 +24,7 @@ fn slice_to_vec<T: FieldElement>(arr: &[i32]) -> Vec<T> {
 fn sqrt_asm() {
    let f = "asm/sqrt.asm";
    let i = [3];
-    regular_test(f, &i);
+    regular_test_without_babybear(f, &i);


why without_babybear again?

since I added the change in your other comment above to also make proofs with p3, some asm tests fail because they have lookups/permutations inside the machine which we don't support

pacheco · 2024-10-09T19:29:54Z

riscv/src/continuations.rs

@@ -216,6 +216,7 @@ pub struct DryRunResult<F: FieldElement> {
 /// - The number of rows after which the prover should jump to the shutdown routine.
 pub fn rust_continuations_dry_run<F: FieldElement>(
    pipeline: &mut Pipeline<F>,
+    field: KnownField,


can get KnownField from F

yea but I wanted to keep them decoupled on purpose, in case we manage to remove F here. But I'll remove it

pacheco · 2024-10-09T19:45:54Z

riscv/src/lib.rs

+    }
+
+    pub fn with_arith(self) -> Self {
+        Self {


you can do the following:

Self { arith: true, ..self }

and it will move all the other fields to the new object

pacheco · 2024-10-09T19:46:58Z

riscv/src/lib.rs

+    }
+
+    pub fn with_continuations(self) -> Self {
+        Self {


same for these

pacheco · 2024-10-09T19:53:54Z

riscv/tests/instructions.rs

    use test_log::test;

    fn run_instruction_test(path: &Path) {
+        let options_32 = CompilerOptions::new_32();
+        run_instruction_test_with_options(path, options_32);


is the extra function needed? function itself is a single statement

in the riscv-bb PR we also run for babybear here, then it made sense. I removed it after the extraction to keep this PR GL only

pacheco · 2024-10-09T19:57:12Z

riscv/tests/riscv.rs

@@ -13,13 +12,12 @@ use test_log::test;

 use powdr_riscv::{
    continuations::{rust_continuations, rust_continuations_dry_run},
-    Runtime,
+    CompilerOptions, RuntimeLibs,
 };

 /// Compiles and runs a rust program with continuations, runs the full
 /// witness generation & verifies it using Pilcom.
 pub fn test_continuations(case: &str) {


should these tests be _64? or should we also have BB inside here?

or maybe _gl?

eventually we should have bb as well, but for now only 64. Do you mean the test name?

leonardoalt commented Oct 8, 2024

View reviewed changes

leonardoalt force-pushed the extract-codegen branch 3 times, most recently from fbd7196 to 5f2bd16 Compare October 8, 2024 14:06

compiler options and codegen split

ae84d1e

leonardoalt force-pushed the extract-codegen branch from 5f2bd16 to ae84d1e Compare October 8, 2024 14:55

pacheco reviewed Oct 9, 2024

View reviewed changes

riscv/src/lib.rs

}

pub fn with_continuations(self) -> Self {

Self {

Copy link

Collaborator

pacheco Oct 9, 2024

Choose a reason for hiding this comment

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

same for these

leonardoalt reacted with thumbs up emoji

pacheco reviewed Oct 9, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

compiler options and codegen split #1879

compiler options and codegen split #1879

leonardoalt commented Oct 8, 2024

leonardoalt Oct 8, 2024

leonardoalt Oct 8, 2024

leonardoalt Oct 8, 2024

leonardoalt Oct 8, 2024

leonardoalt Oct 8, 2024

leonardoalt Oct 8, 2024

leonardoalt Oct 8, 2024

pacheco Oct 9, 2024

leonardoalt Oct 9, 2024

pacheco Oct 9, 2024

pacheco Oct 9, 2024

pacheco Oct 9, 2024 •

edited

Loading

leonardoalt Oct 9, 2024

pacheco Oct 9, 2024

leonardoalt Oct 9, 2024

pacheco Oct 9, 2024

leonardoalt Oct 9, 2024

pacheco Oct 9, 2024

pacheco Oct 9, 2024

pacheco Oct 9, 2024 •

edited

Loading

leonardoalt Oct 9, 2024

pacheco Oct 9, 2024

pacheco Oct 9, 2024

leonardoalt Oct 9, 2024

compiler options and codegen split #1879

Are you sure you want to change the base?

compiler options and codegen split #1879

Conversation

leonardoalt commented Oct 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pacheco Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pacheco Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pacheco Oct 9, 2024 •

edited

Loading

pacheco Oct 9, 2024 •

edited

Loading