Powdr/RISCV executor #679

lvella · 2023-10-06T16:41:34Z

No description provided.

leonardoalt · 2023-10-18T12:49:10Z

powdr_cli/Cargo.toml

@@ -4,7 +4,7 @@ version = "0.1.0"
 edition = "2021"

 [features]
-default = [] # halo2 is disabled by default
+default = []                                             # halo2 is disabled by default


why the extra space?

Code formatter did this.

Can you undo it?

I asked chatgpt how to disable code formatting for Cargo.toml.

leonardoalt · 2023-10-18T13:21:21Z

analysis/src/vm/mod.rs

-    // remove all asm (except external instructions)
-    log::debug!("Run asm_to_pil analysis step");
-    let file = asm_to_pil::compile(file);
-    monitor.push(&file);


This is the best point to perform the execution, so the last step was separated into another function.

leonardoalt · 2023-10-18T13:22:09Z

riscv_executor/src/lib.rs

+use number::{BigInt, FieldElement};
+
+#[derive(Clone, Copy)]
+pub union Elem {


why not an enum?

Because the same binary string must be interpreted as either unsigned or signed, depending on the context.

leonardoalt · 2023-10-18T13:26:34Z

There must be something weird going on: I ran this on an empty Rust main function, and at the end the trace.reg_map is:

Executing...
{"addr": 41, "x7": 16, "x25": 34, "x0": 9, "x19": 28, "x23": 32, "lr_sc_reservation": 8, "x5": 14, "x28": 37, "x11": 20, "Y": 2, "x30": 39, "x31": 40, "x4": 13, "x2": 11, "x13": 22, "x15": 24, "W": 4, "x18": 27, "x20": 29, "x12": 21, "pc": 0, "x27": 36, "tmp2": 6, "x6": 15, "Z": 3, "x9": 18, "x29": 38, "x17": 26, "x21": 30, "x8": 17, "x10": 19, "x16": 25, "x22": 31, "x24": 33, "x1": 10, "tmp1": 5, "tmp3": 7, "x3": 12, "x14": 23, "x26": 35, "X": 1}

A few weird things:

x0 should always be 0
actually all registers should be 0
pc should be something like 100 or so
probably more?

maybe I just don't understand yet how this works

leonardoalt · 2023-10-18T13:43:25Z

    function main {
		ret;
    }

If I run it on this it seems to get into an infinite loop and runs OOM.

leonardoalt · 2023-10-18T15:14:58Z

(rebased)

leonardoalt · 2023-10-18T16:13:44Z

maybe I just don't understand yet how this works

ok it looks like reg_map is a map from register names to indices in the table.

leonardoalt · 2023-10-18T16:34:39Z

Ok if I print everything at the end properly it seems correct, although I don't know why pc=1 at the end:

pc=0 X=0 Y=0 Z=0 W=0 tmp1=0 tmp2=0 tmp3=0 lr_sc_reservation=0 x0=0 x1=0 x2=0 x3=0 x4=0 x5=0 x6=0 x7=0 x8=0 x9=0 x10=0 x11=0 x12=0 x13=0 x14=0 x15=0 x16=0 x17=0 x18=0 x19=0 x20=0 x21=0 x22=0 x23=0 x24=0 x25=0 x26=0 x27=0 x28=0 x29=0 x30=0 x31=0 addr=0

pc=0 X=0 Y=0 Z=0 W=0 tmp1=0 tmp2=0 tmp3=0 lr_sc_reservation=0 x0=0 x1=0 x2=0 x3=0 x4=0 x5=0 x6=0 x7=0 x8=0 x9=0 x10=0 x11=0 x12=0 x13=0 x14=0 x15=0 x16=0 x17=0 x18=0 x19=0 x20=0 x21=0 x22=0 x23=0 x24=0 x25=0 x26=0 x27=0 x28=0 x29=0 x30=0 x31=0 addr=0

pc=1 X=0 Y=0 Z=0 W=0 tmp1=0 tmp2=0 tmp3=0 lr_sc_reservation=0 x0=0 x1=0 x2=0 x3=0 x4=0 x5=0 x6=0 x7=0 x8=0 x9=0 x10=0 x11=0 x12=0 x13=0 x14=0 x15=0 x16=0 x17=0 x18=0 x19=0 x20=0 x21=0 x22=0 x23=0 x24=0 x25=0 x26=0 x27=0 x28=0 x29=0 x30=0 x31=0 addr=0

lvella · 2023-10-23T07:39:14Z

Ok if I print everything at the end properly it seems correct, although I don't know why pc=1 at the end:

pc=0 X=0 Y=0 Z=0 W=0 tmp1=0 tmp2=0 tmp3=0 lr_sc_reservation=0 x0=0 x1=0 x2=0 x3=0 x4=0 x5=0 x6=0 x7=0 x8=0 x9=0 x10=0 x11=0 x12=0 x13=0 x14=0 x15=0 x16=0 x17=0 x18=0 x19=0 x20=0 x21=0 x22=0 x23=0 x24=0 x25=0 x26=0 x27=0 x28=0 x29=0 x30=0 x31=0 addr=0

pc=0 X=0 Y=0 Z=0 W=0 tmp1=0 tmp2=0 tmp3=0 lr_sc_reservation=0 x0=0 x1=0 x2=0 x3=0 x4=0 x5=0 x6=0 x7=0 x8=0 x9=0 x10=0 x11=0 x12=0 x13=0 x14=0 x15=0 x16=0 x17=0 x18=0 x19=0 x20=0 x21=0 x22=0 x23=0 x24=0 x25=0 x26=0 x27=0 x28=0 x29=0 x30=0 x31=0 addr=0

pc=1 X=0 Y=0 Z=0 W=0 tmp1=0 tmp2=0 tmp3=0 lr_sc_reservation=0 x0=0 x1=0 x2=0 x3=0 x4=0 x5=0 x6=0 x7=0 x8=0 x9=0 x10=0 x11=0 x12=0 x13=0 x14=0 x15=0 x16=0 x17=0 x18=0 x19=0 x20=0 x21=0 x22=0 x23=0 x24=0 x25=0 x26=0 x27=0 x28=0 x29=0 x30=0 x31=0 addr=0

I don't understand why there are 2 pc=0. pc is incremented on every instruction.

leonardoalt · 2023-10-23T10:45:41Z

I don't understand why there are 2 pc=0. pc is incremented on every instruction.

Each line is a row

chriseth · 2023-10-25T15:29:00Z

analysis/src/lib.rs

+    Ok(file)
+}
+
+pub fn consume_asm<T: FieldElement>(


consume is kind of weird because it does not say what it converts it to.
What about calling this convert_analyzed_to_pil and the other convert_asm_to_pil or something?

chriseth · 2023-10-25T15:39:43Z

riscv_executor/src/lib.rs

+        }
+
+        /// set next value of register
+        pub(crate) fn s(&mut self, idx: &str, value: impl Into<Elem>) {


Wouldn't it be much better readable if s would return a mutable reference to the value?

In my subjective opinion, no.

Instead of:

obj.s("pc", x + 42);

it would be

*obj.s("pc") = (x + 42).into();

or more conventionally

*obj.g_mut("pc") = (x + 42).into();

But most importantly, I can't have the logic to handle PC or x0 if I return a mut reference.

leonardoalt · 2023-11-03T10:43:26Z

rebased, last commit needs to be doubled checked because of some AST changes

chriseth · 2023-11-13T11:16:40Z

riscv_executor/src/lib.rs

+            "jump_and_link_dyn" => {
+                let pc = self.proc.g("pc");
+                self.proc.s("x1", pc.u() + 1);
+                self.proc.s("pc", args[0]);


Maybe also add a set_pc function? Is it actually legal to set the PC in a regular instruction? If not, we could remove the special case in the s function.

You mean, like in addi pc,pc,42? Some random interwebs slides says it is not possible. I'll do it.

But still find the s(key, value) solution more elegant than returning mut &, and I still need to handle the special case of x0.

leonardoalt · 2023-11-24T09:56:28Z

rebased & squashed

lvella force-pushed the riscv-executor-tmp-base branch from b53d14f to 22f76ec Compare October 11, 2023 14:51

lvella force-pushed the riscv-executor branch 2 times, most recently from b4071e1 to d2a1bf3 Compare October 13, 2023 17:27

lvella force-pushed the riscv-executor-tmp-base branch from 91d16b6 to 5731b7b Compare October 13, 2023 17:34

lvella force-pushed the riscv-executor branch from d2a1bf3 to c431978 Compare October 13, 2023 17:34

leonardoalt reviewed Oct 18, 2023

View reviewed changes

leonardoalt force-pushed the riscv-executor branch from d6443b2 to cd23c52 Compare October 18, 2023 15:14

lvella force-pushed the riscv-executor-tmp-base branch from 5731b7b to 9130a65 Compare October 23, 2023 08:02

chriseth reviewed Oct 25, 2023

View reviewed changes

lvella force-pushed the riscv-executor branch from 8c86254 to 883d55e Compare October 26, 2023 13:14

lvella changed the base branch from riscv-executor-tmp-base to main October 26, 2023 13:15

lvella force-pushed the riscv-executor branch 2 times, most recently from d9ecec4 to a08fc5a Compare October 26, 2023 18:17

leonardoalt force-pushed the riscv-executor branch from 452dbd1 to 5862cef Compare November 3, 2023 10:43

leonardoalt force-pushed the riscv-executor branch 3 times, most recently from a0df34f to 9ca3665 Compare November 6, 2023 19:36

lvella force-pushed the riscv-executor branch 4 times, most recently from ce31e9f to 80e2f87 Compare November 10, 2023 16:54

chriseth reviewed Nov 13, 2023

View reviewed changes

lvella marked this pull request as ready for review November 16, 2023 17:18

lvella force-pushed the riscv-executor branch 11 times, most recently from f4c5077 to 070b6df Compare November 20, 2023 18:26

leonardoalt force-pushed the riscv-executor branch 2 times, most recently from feeb9ba to edd1f10 Compare November 24, 2023 09:56

leonardoalt force-pushed the riscv-executor branch from edd1f10 to 0b374b1 Compare November 24, 2023 10:05

leonardoalt previously approved these changes Nov 24, 2023

View reviewed changes

leonardoalt enabled auto-merge November 24, 2023 10:06

RISCV executor

cd4449e

leonardoalt dismissed their stale review via cd4449e November 24, 2023 10:47

leonardoalt force-pushed the riscv-executor branch from 0b374b1 to cd4449e Compare November 24, 2023 10:47

leonardoalt approved these changes Nov 24, 2023

View reviewed changes

leonardoalt added this pull request to the merge queue Nov 24, 2023

Merged via the queue into main with commit 99979a8 Nov 24, 2023
2 checks passed

leonardoalt deleted the riscv-executor branch November 24, 2023 11:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Powdr/RISCV executor #679

Powdr/RISCV executor #679

lvella commented Oct 6, 2023

leonardoalt Oct 18, 2023

lvella Oct 23, 2023

chriseth Oct 25, 2023

lvella Nov 10, 2023

leonardoalt Oct 18, 2023

lvella Oct 23, 2023

leonardoalt Oct 18, 2023

lvella Oct 23, 2023

leonardoalt Oct 23, 2023

leonardoalt commented Oct 18, 2023

leonardoalt commented Oct 18, 2023

leonardoalt commented Oct 18, 2023

leonardoalt commented Oct 18, 2023

leonardoalt commented Oct 18, 2023

lvella commented Oct 23, 2023

leonardoalt commented Oct 23, 2023

chriseth Oct 25, 2023

chriseth Oct 25, 2023

lvella Oct 25, 2023

leonardoalt commented Nov 3, 2023

chriseth Nov 13, 2023

lvella Nov 13, 2023 •

edited

Loading

lvella Nov 13, 2023

leonardoalt commented Nov 24, 2023

Powdr/RISCV executor #679

Powdr/RISCV executor #679

Conversation

lvella commented Oct 6, 2023

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

leonardoalt commented Oct 18, 2023

leonardoalt commented Oct 18, 2023

leonardoalt commented Oct 18, 2023

leonardoalt commented Oct 18, 2023

leonardoalt commented Oct 18, 2023

lvella commented Oct 23, 2023

leonardoalt commented Oct 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leonardoalt commented Nov 3, 2023

Choose a reason for hiding this comment

lvella Nov 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leonardoalt commented Nov 24, 2023

lvella Nov 13, 2023 •

edited

Loading