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.

Sign up for GitHub

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

emul-branch: move branch logic in its own category #512

Merged
merged 1 commit into from
Oct 31, 2024
Merged

emul-branch: move branch logic in its own category #512

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 44 additions & 21 deletions ceno_emul/src/rv32im.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ pub struct DecodedInstruction {
#[derive(Clone, Copy, Debug)]
pub enum InsnCategory {
Compute,
Branch,
Load,
Store,
System,
Expand Down Expand Up @@ -418,12 +419,12 @@ const RV32IM_ISA: InstructionTable = [
insn(I, SRAI, Compute, 0x13, 0x5, 0x20),
insn(I, SLTI, Compute, 0x13, 0x2, -1),
insn(I, SLTIU, Compute, 0x13, 0x3, -1),
insn(B, BEQ, Compute, 0x63, 0x0, -1),
insn(B, BNE, Compute, 0x63, 0x1, -1),
insn(B, BLT, Compute, 0x63, 0x4, -1),
insn(B, BGE, Compute, 0x63, 0x5, -1),
insn(B, BLTU, Compute, 0x63, 0x6, -1),
insn(B, BGEU, Compute, 0x63, 0x7, -1),
insn(B, BEQ, Branch, 0x63, 0x0, -1),
insn(B, BNE, Branch, 0x63, 0x1, -1),
insn(B, BLT, Branch, 0x63, 0x4, -1),
insn(B, BGE, Branch, 0x63, 0x5, -1),
insn(B, BLTU, Branch, 0x63, 0x6, -1),
insn(B, BGEU, Branch, 0x63, 0x7, -1),
insn(J, JAL, Compute, 0x6f, -1, -1),
insn(I, JALR, Compute, 0x67, 0x0, -1),
insn(U, LUI, Compute, 0x37, -1, -1),
Expand Down Expand Up @@ -556,6 +557,7 @@ impl Emulator {

if match insn.category {
InsnCategory::Compute => self.step_compute(ctx, insn.kind, &decoded)?,
InsnCategory::Branch => self.step_branch(ctx, insn.kind, &decoded)?,
InsnCategory::Load => self.step_load(ctx, insn.kind, &decoded)?,
InsnCategory::Store => self.step_store(ctx, insn.kind, &decoded)?,
InsnCategory::System => self.step_system(ctx, insn.kind, &decoded)?,
Expand All @@ -577,15 +579,7 @@ impl Emulator {

let pc = ctx.get_pc();
let mut new_pc = pc + WORD_SIZE;
let mut rd = decoded.rd;
let imm_i = decoded.imm_i();
let mut br_cond = |cond| -> u32 {
rd = 0;
if cond {
new_pc = pc.wrapping_add(decoded.imm_b());
}
0
};
let out = match kind {
// Instructions that do not read rs1 nor rs2.
JAL => {
Expand Down Expand Up @@ -653,12 +647,6 @@ impl Emulator {
0
}
}
BEQ => br_cond(rs1 == rs2),
BNE => br_cond(rs1 != rs2),
BLT => br_cond((rs1 as i32) < (rs2 as i32)),
BGE => br_cond((rs1 as i32) >= (rs2 as i32)),
BLTU => br_cond(rs1 < rs2),
BGEU => br_cond(rs1 >= rs2),
MUL => rs1.wrapping_mul(rs2),
MULH => {
(sign_extend_u32(rs1).wrapping_mul(sign_extend_u32(rs2)) >> 32)
Expand Down Expand Up @@ -704,7 +692,42 @@ impl Emulator {
if !new_pc.is_aligned() {
return ctx.trap(TrapCause::InstructionAddressMisaligned);
}
ctx.store_register(rd as usize, out)?;
ctx.store_register(decoded.rd as usize, out)?;
ctx.set_pc(new_pc);
Ok(true)
}

fn step_branch<M: EmuContext>(
&self,
ctx: &mut M,
kind: InsnKind,
decoded: &DecodedInstruction,
) -> Result<bool> {
use InsnKind::*;

let pc = ctx.get_pc();
let rs1 = ctx.load_register(decoded.rs1 as RegIdx)?;
let rs2 = ctx.load_register(decoded.rs2 as RegIdx)?;

let taken = match kind {
BEQ => rs1 == rs2,
BNE => rs1 != rs2,
BLT => (rs1 as i32) < (rs2 as i32),
BGE => (rs1 as i32) >= (rs2 as i32),
BLTU => rs1 < rs2,
BGEU => rs1 >= rs2,
_ => unreachable!("Illegal branch instruction: {:?}", kind),
};

let new_pc = if taken {
pc.wrapping_add(decoded.imm_b())
Copy link
Collaborator

Choose a reason for hiding this comment

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

You could change this to something like:

let new_pc = pc.wrapping_add(if taken { decoded.imm_b() } else { WORD_SIZE })

Btw, in my old VM we let the decoder change all the branches from relative addressing to absolute addressing, and forbid having executable code in the last four bytes, so we didn't have to worry about wrap-around for branches.

(If you are happy to forbid a few more kb of memory for execution, you can leave the relative addressing in, but still not worry about wrap-around.)

Wrap-around ain't a problem for the emulator, only for the constraints. See the discussion we had about memory.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I just didn’t want to change any behavior in this PR.

Indeed, wrap-around by WORD_SIZE increment is not supported (unsatisfiable).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, preserving behaviour in this PR is a good idea. I was talking more generally, but unfortunately didn't make that clear. Sorry.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Would you be able to make a follow up PR with the changes we discussed perhaps?

} else {
pc + WORD_SIZE
};

if !new_pc.is_aligned() {
return ctx.trap(TrapCause::InstructionAddressMisaligned);
}
ctx.set_pc(new_pc);
Ok(true)
}
Expand Down
Loading