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

sync-platform: Configure the platform / emulator to match circuits #735

Merged
merged 7 commits into from
Dec 13, 2024

Conversation

naure
Copy link
Collaborator

@naure naure commented Dec 12, 2024

Over time we moved from a first version (#215) which looked end-to-end like this:

Static memory layout 
            `-->  Program compilation
            `-->  Trace generation
            `-->  Static circuits

to this

Existing third-party programs
            `-->  Auto-detected memory layout
                        `-->  Trace generation
                        `-->  Configurable circuits

This worked by letting the trace generation be more powerful and more permissive than the circuits (larger memory ranges, etc).

This PR configures the trace generation to tightly match the circuit behaviors as of today.

@matthiasgoergens
Copy link
Collaborator

Could you please expand the description with a short note on what aspects you are syncing up, please?

Copy link
Collaborator

@matthiasgoergens matthiasgoergens left a comment

Choose a reason for hiding this comment

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

Overall a nice consolidation and simplification!

Despite the title of the PR, I didn't see all that much new synchronisation between the platform and emulator here? Perhaps I'm missing something. Could you please elaborate in the PR description what new aspects we are matching that we didn't match before? Thanks!

(I do mostly like the changes here in any case, even if I don't quite understand the title.)

ceno_emul/src/platform.rs Outdated Show resolved Hide resolved
ceno_emul/src/platform.rs Show resolved Hide resolved
ceno_emul/src/platform.rs Show resolved Hide resolved
ceno_emul/src/platform.rs Outdated Show resolved Hide resolved
ceno_emul/src/platform.rs Outdated Show resolved Hide resolved
ceno_emul/src/platform.rs Show resolved Hide resolved
ceno_zkvm/src/e2e.rs Show resolved Hide resolved
ceno_zkvm/src/e2e.rs Show resolved Hide resolved
@matthiasgoergens
Copy link
Collaborator

Please mention that we are doing this partially for SP1 compatibility in the PR description.

Copy link
Collaborator

@matthiasgoergens matthiasgoergens left a comment

Choose a reason for hiding this comment

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

Please see comments.

@naure naure enabled auto-merge December 13, 2024 06:50
@naure naure added this pull request to the merge queue Dec 13, 2024
github-merge-queue bot pushed a commit that referenced this pull request Dec 13, 2024
)

Over time we moved from a first version
(#215) which looked end-to-end
like this:

```
Static memory layout 
            `-->  Program compilation
            `-->  Trace generation
            `-->  Static circuits
```

to this

```
Existing third-party programs
            `-->  Auto-detected memory layout
                        `-->  Trace generation
                        `-->  Configurable circuits
```

This worked by letting the trace generation be more powerful and more
permissive than the circuits (larger memory ranges, etc).

This PR configures the trace generation to tightly match the circuit
behaviors as of today.

---------

Co-authored-by: Aurélien Nicolas <[email protected]>
@matthiasgoergens matthiasgoergens removed this pull request from the merge queue due to a manual request Dec 13, 2024
Copy link
Collaborator

@matthiasgoergens matthiasgoergens left a comment

Choose a reason for hiding this comment

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

Sorry, I prematurely removed my hold. Please just use a BTreeSet or at least a /// doc comment.

@@ -10,20 +10,23 @@ use crate::addr::{Addr, RegIdx};
#[derive(Clone, Debug)]
pub struct Platform {
pub rom: Range<Addr>,
pub ram: Range<Addr>,
// This is an `Option` to allow `const` here.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// This is an `Option` to allow `const` here.
/// This is an `Option` to allow `pub const CENO_PLATFORM: Platform`

@matthiasgoergens matthiasgoergens dismissed their stale review December 13, 2024 07:49

Only a minor cosmetic request.

@matthiasgoergens matthiasgoergens added this pull request to the merge queue Dec 13, 2024
@matthiasgoergens
Copy link
Collaborator

I enabled auto-merge again. (It got disabled, when I mucked around with my review.)

@matthiasgoergens
Copy link
Collaborator

@naure Sorry, the renaming of ROM to program code (or similar) is fine. And I recognise that the changes you made to the can_write function is also fine in the sense that they make the emulator reflect what the circuits currently enforce. (A promised in the PR description.)

My request to respect read-only memory is more an aspiration for the future development.

@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Dec 13, 2024
@matthiasgoergens matthiasgoergens added this pull request to the merge queue Dec 13, 2024
Merged via the queue into scroll-tech:master with commit 0cd4234 Dec 13, 2024
6 checks passed
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.

Decide and implement details of platform, memory layout, and I/O
3 participants