Skip to content

spec: rework CPU for smaller footprint#624

Merged
RobinJadoul merged 25 commits into
spec/mainfrom
spec/shrink-cpu
Jun 2, 2026
Merged

spec: rework CPU for smaller footprint#624
RobinJadoul merged 25 commits into
spec/mainfrom
spec/shrink-cpu

Conversation

@RobinJadoul
Copy link
Copy Markdown
Collaborator

No description provided.

@RobinJadoul RobinJadoul self-assigned this May 26, 2026
@RobinJadoul RobinJadoul added the spec Updates and improvements to the spec document label May 26, 2026
@RobinJadoul RobinJadoul marked this pull request as ready for review May 27, 2026 11:59
@RobinJadoul RobinJadoul requested a review from erik-3milabs May 27, 2026 11:59
@github-actions
Copy link
Copy Markdown

Codex Code Review

Findings

  • High - word-instruction rows can choose an unconstrained outer instruction_length. In spec/src/cpu.toml, the outer DECODE lookup is disabled when word_instr = 1, and the delegation to CPU32 only passes timestamp and pc at spec/src/cpu.toml. But the outer CPU still computes next_pc from its own instruction_length at spec/src/cpu.toml. Since that value is not linked to the CPU32 decode row, a *W instruction proof can advance the PC by an arbitrary byte value while still satisfying the delegated CPU32 execution. This is a VM soundness issue. Pass the decoded instruction length through the CPU32 interaction or keep the outer decode lookup active enough to bind it.

  • High - CPU32 uses rv1 instead of rv2 when building arg2. The constraint text says arg2[0] = rv2[:2] + imm[0], but the polynomial subtracts rv1 at spec/src/cpu32.toml. That makes ADDW/SUBW/MULW/DIVW/... with an rs2 operand use rs1 as both operands, and immediate *W instructions use rs1 + imm as the second operand. Replace the rv1 cast with rv2.

  • Medium - conditional branches appear to compare against instruction_length instead of rs2. For BRANCH && !JALR, arg2 is set to instruction_length at spec/src/cpu.toml, then the ALU uses rv1, arg2 at spec/src/cpu.toml. For BEQ/BLT-style branches this should compare rs1 with rs2; otherwise branch conditions are evaluated against 2/4 instead of the second register.

No other concrete issues found in the reviewed diff.

Copy link
Copy Markdown
Collaborator

@erik-3milabs erik-3milabs left a comment

Choose a reason for hiding this comment

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

Note to self for follow-up review: have a closer look at the interplay between CPU and DECODE padding.

Comment thread spec/decode.typ Outdated
Comment thread spec/src/decode_uncompressed.toml Outdated
Comment thread spec/src/decode_uncompressed.toml
Comment thread spec/cpu.typ Outdated
Comment thread spec/src/cpu.toml Outdated
Comment thread spec/src/store.toml
Comment thread spec/src/signatures.toml
Comment thread spec/src/cpu.toml Outdated
Comment thread spec/src/signatures.toml
Comment thread spec/src/mul.toml
RobinJadoul and others added 2 commits May 29, 2026 13:13
Co-authored-by: Erik <159244975+erik-3milabs@users.noreply.github.com>
@RobinJadoul RobinJadoul requested a review from erik-3milabs May 29, 2026 12:26
@RobinJadoul
Copy link
Copy Markdown
Collaborator Author

RobinJadoul commented May 29, 2026

Maybe 9b30b9e doesn't belong in this PR exactly, but it seemed simple enough for me to not want to deal with having a stacked PR for it :)

Comment thread spec/src/cpu.toml Outdated
Comment thread spec/src/cpu.toml
Comment thread spec/src/cpu.toml
Comment thread spec/src/cpu32.toml Outdated
Comment thread spec/src/cpu.toml Outdated
Comment thread spec/src/load.toml Outdated
Comment thread spec/src/shift.toml
Comment thread spec/src/signatures.toml
Comment thread spec/dvrm.typ Outdated
Comment thread spec/cpu.typ Outdated
- For the `BRANCH` path, when we do not have a JALR instruction, we want the ALU output to reflect the branch condition.
- For the `BRANCH` path, when it is a JALR instruction, we want the ALU output to contain the next instructions PC value, to store into `rd`.

Instructions having the `word_instr` flag set are delegated to the `CPU32` chip, which will do its own decoding and execution of the instruction.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I believe this sentence is out-of-date now.

Suggested change
Instructions having the `word_instr` flag set are delegated to the `CPU32` chip, which will do its own decoding and execution of the instruction.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

How so? We still have the CPU32 interaction for this

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

That is correct. However, this sentence is in the CPU description twice: the new one you provided when you moved the CPU32 interaction up, and this (old) one that does not add any value here.

@RobinJadoul RobinJadoul requested a review from erik-3milabs June 1, 2026 15:42
Comment thread spec/src/cpu.toml Outdated
Comment thread spec/src/cpu.toml Outdated
Comment thread spec/src/cpu.toml Outdated
Comment thread spec/cpu.typ Outdated
- For the `BRANCH` path, when we do not have a JALR instruction, we want the ALU output to reflect the branch condition.
- For the `BRANCH` path, when it is a JALR instruction, we want the ALU output to contain the next instructions PC value, to store into `rd`.

Instructions having the `word_instr` flag set are delegated to the `CPU32` chip, which will do its own decoding and execution of the instruction.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

That is correct. However, this sentence is in the CPU description twice: the new one you provided when you moved the CPU32 interaction up, and this (old) one that does not add any value here.

RobinJadoul and others added 2 commits June 2, 2026 15:04
Co-authored-by: Erik <159244975+erik-3milabs@users.noreply.github.com>
Copy link
Copy Markdown
Collaborator

@erik-3milabs erik-3milabs left a comment

Choose a reason for hiding this comment

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

Green light!

@RobinJadoul RobinJadoul merged commit f006292 into spec/main Jun 2, 2026
2 checks passed
@RobinJadoul RobinJadoul deleted the spec/shrink-cpu branch June 2, 2026 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

spec Updates and improvements to the spec document

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants