Add RISC-V compressed instruction support

[gabrielbosio]

No description provided.

[github-actions]

Codex Code Review

Findings

• Medium - executor can panic or wrap on top-of-address-space 4-byte fetch
  executor/src/vm/execution.rs now supports pc % 4 == 2, but the on-demand 4-byte path does self.pc + 2 unchecked. A valid even PC such as u64::MAX - 1 can be reached via jalr; if the low halfword has low bits 0b11, debug builds panic on overflow and release builds wrap the high-half fetch to address 1. Use checked_add(2) and return MemoryError::AddressOverflow/ExecutorError instead.

Verification

I could not run the targeted cargo tests because Rustup attempted to write under the read-only home directory:

error: could not create temp file /home/runner/.rustup/tmp/...: Read-only file system

[github-actions]

Benchmark Results for unmodified programs 🚀

Command	Mean [ms]	Min [ms]	Max [ms]	Relative
base binary_search	58.9 ± 1.4	57.4	61.6	1.00
head binary_search	60.3 ± 1.2	58.7	62.0	1.02 ± 0.03

Command	Mean [ms]	Min [ms]	Max [ms]	Relative
base bitwise_ops	58.5 ± 1.4	56.7	60.6	1.00
head bitwise_ops	59.5 ± 1.9	57.9	62.9	1.02 ± 0.04

Command	Mean [ms]	Min [ms]	Max [ms]	Relative
base fibonacci_26	63.1 ± 1.5	61.5	65.6	1.00 ± 0.03
head fibonacci_26	63.0 ± 0.9	62.2	64.6	1.00

Command	Mean [ms]	Min [ms]	Max [ms]	Relative
base hashmap	135.2 ± 3.1	132.4	142.1	1.00
head hashmap	143.7 ± 2.3	141.2	147.1	1.06 ± 0.03

Command	Mean [ms]	Min [ms]	Max [ms]	Relative
base keccak	129.7 ± 3.5	124.0	134.8	1.00
head keccak	141.0 ± 4.0	130.9	145.4	1.09 ± 0.04

Command	Mean [ms]	Min [ms]	Max [ms]	Relative
base matrix_multiply	63.5 ± 0.4	62.7	64.0	1.00
head matrix_multiply	65.3 ± 1.2	63.8	67.4	1.03 ± 0.02

Command	Mean [ms]	Min [ms]	Max [ms]	Relative
base modular_exp	59.0 ± 1.6	57.4	61.7	1.00
head modular_exp	61.1 ± 1.9	59.3	63.9	1.04 ± 0.04

Command	Mean [ms]	Min [ms]	Max [ms]	Relative
base quicksort	62.9 ± 0.9	61.7	64.9	1.00
head quicksort	64.4 ± 0.8	63.0	65.2	1.02 ± 0.02

Command	Mean [ms]	Min [ms]	Max [ms]	Relative
base sieve	64.9 ± 1.0	63.9	66.5	1.00
head sieve	65.3 ± 1.0	64.4	66.9	1.01 ± 0.02

Command	Mean [ms]	Min [ms]	Max [ms]	Relative
base sum_array	74.5 ± 1.0	73.0	76.2	1.00
head sum_array	75.2 ± 0.7	74.1	76.1	1.01 ± 0.02

Command	Mean [ms]	Min [ms]	Max [ms]	Relative
base syscall_commit	91.9 ± 2.3	89.8	94.8	1.00
head syscall_commit	93.1 ± 2.0	91.1	95.9	1.01 ± 0.03

[claude]

Low – Unreachable arm (dead code)

The match on ((h >> 12) & 1, rs2) is exhaustive: the first element is 1-bit (0 or 1), and (0, 0) / (0, _) / (1, 0) / (1, _) cover every possible tuple. The _ => arm can never be reached. Consider replacing it with unreachable!() so the compiler can flag any future gap, rather than silently swallowing it as a valid error path:

Suggested change

	_ => Err(InstructionError::IllegalCompressed(half)),
	_ => unreachable!("all CR patterns covered"),

[claude]

Low – Weak negative assertion

The original test positively asserted InstructionAddressMisaligned(2), which provided a precise contract. The replacement only asserts the error is not a misalignment trap — any other ExecutorError (including hypothetical future bugs producing the wrong variant) would satisfy it. Consider adding a positive assertion of the expected error kind, e.g.:

assert!(
    matches!(
        err,
        ExecutorError::MemoryError(_)
            | ExecutorError::InstructionError(_)
            | ExecutorError::InvalidInstruction(_)
    ),
    "expected a decode/memory error at pc=2, got {:?}",
    err
);

(Use whichever ExecutorError variant is produced by a bad decode at that address.)

[claude]

Medium – Silent skip creates prover/executor asymmetry for c.unimp

Skipping 0x0000 is logically correct (it can never be the first halfword of a 4-byte instruction since bits[1:0] would need to be 11), but the behaviour differs between paths:

• Segment pre-decode (this path): the address is not inserted into the DECODE table.
• On-demand fetch (execution.rs, lo = 0x0000): calls decompress(0x0000) → IllegalCompressed.

This means a jump to a c.unimp in a pre-decoded segment hits the on-demand path at runtime (cache miss → fetch → error), which is correct. However, any ELF where the compiler deliberately emits c.unimp as a trap instruction will silently drop those addresses from the instruction cache. If the prover ever needs to prove an execution that touches such an address (it can't — the executor would error first), the DECODE table wouldn't have the row.

The current behaviour is safe, but it's worth noting: decode_segment_words and decompress treat 0x0000 differently (skip vs. error). A future change to pre-populate the DECODE table for all reachable addresses (including traps) would need to revisit this skip.

[claude]

Review: Add RV64C compressed instruction support

Overall: Well-implemented. The bit-manipulation for immediate decoding was verified against the RISC-V unprivileged ISA spec tables — all scramble patterns are correct (CI, CJ, CB, CL/CS immediates). The prover integration (c_type flag, single decode_segment_words entry point shared by executor cache and prover/verifier) prevents the two sides from ever disagreeing on instruction boundaries. Test coverage is solid.

---

Issues found

Low – Dead code in decompress_q2_cr (inline comment on decompress.rs:371)
The _ => Err(...) arm is unreachable: the match tuple ((h >> 12) & 1, rs2) is covered exhaustively by (0,0), (0,_), (1,0), (1,_). Should be unreachable!() so the compiler can catch any future gap.

Low – Weak negative assertion in test_2byte_aligned_pc_not_misaligned (inline comment on asm.rs:522)
The original test positively asserted the specific error variant; the replacement only asserts it is not a misalignment error. Worth tightening to a positive match on the expected decode/memory error variant.

Medium – Silent skip of 0x0000 in decode_segment_words (inline comment on decoding.rs:333)
Logically safe (0x0000 can never start a 4-byte instruction, bits[1:0] ≠ 11), and the asymmetry with the on-demand fetch path is benign today. Noted in case the DECODE table ever needs to account for reachable-trap addresses.

---

Minor observations (no fix needed)

• INSTRUCTION_ALIGN is applied to all PT_LOAD segments (executable and data). For data segments the old 4-byte requirement was just the word size, not an ABI constraint, so relaxing to 2 is harmless — but the constant name is slightly misleading.
• No tests for backward (negative-offset) compressed branches or for C.SUBW/C.ADDW. These code paths look correct from the spec mapping but would add confidence.
• decode_segment_words allocates an intermediate Vec<u16>. Fine for startup-time use; flagging only for future iterator-based refactors.