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

implementation for shift left logical immediate riscv32im #2836

Merged
merged 4 commits into from
Dec 16, 2024

Conversation

svv232
Copy link
Member

@svv232 svv232 commented Nov 25, 2024

No description provided.

@svv232 svv232 requested a review from dannywillems November 25, 2024 17:58
Copy link

codecov bot commented Nov 26, 2024

Codecov Report

Attention: Patch coverage is 29.41176% with 12 lines in your changes missing coverage. Please review.

Project coverage is 72.02%. Comparing base (83e423a) to head (4ba3d58).
Report is 137 commits behind head on master.

Files with missing lines Patch % Lines
o1vm/src/interpreters/riscv32im/interpreter.rs 29.41% 12 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2836      +/-   ##
==========================================
- Coverage   72.04%   72.02%   -0.03%     
==========================================
  Files         256      257       +1     
  Lines       60138    60759     +621     
==========================================
+ Hits        43329    43761     +432     
- Misses      16809    16998     +189     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Base automatically changed from sai/load_half_unsigned_riscv32im to master December 2, 2024 14:54
let local_rs1 = env.read_register(&rs1);
let shamt = {
let pos = env.alloc_scratch();
unsafe { env.bitmask(&imm, 4, 0, pos) }
Copy link
Member

Choose a reason for hiding this comment

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

Is it not 5?

    /// Format: `slli rd, rs1, shamt`
    ///
    /// Description: Performs logical left shift on the value in register rs1 by
    /// the shift amount held in the lower 5 bits of the immediate

Copy link
Member

Choose a reason for hiding this comment

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

See #2865

unsafe { env.bitmask(&imm, 4, 0, pos) }
};
// parse shamt from imm as 20-24 of instruction and 0-4 wrt to imm
let rd_scratch = env.alloc_scratch();
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I would simply keep the same syntax as the others to allocate variables, like:

let local_rd = {
  let pos = env.alloc_scratch();
  unsafe { env.shift_left(&local_rs1, &shamt, rd_scratch) }
};

But it is a very small detail.

let local_rs1 = env.read_register(&rs1);
let shamt = {
let pos = env.alloc_scratch();
unsafe { env.bitmask(&imm, 4, 0, pos) }
Copy link
Member

Choose a reason for hiding this comment

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

There must also be constraints that it is actually 5 bits, and also the 5 lowest bits of the immediate.

Copy link
Member

@dannywillems dannywillems left a comment

Choose a reason for hiding this comment

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

This is incorrect. See comments above.

@dannywillems
Copy link
Member

I would recommend to write some tests on top of #2866. It requires to implement lui, available here and syscall. lui seems to be the first implementation to implement, modulo jal (even though it can be avoided), as it requires to set some registers to some initial values for the syscall.

@svv232 svv232 requested a review from dannywillems December 11, 2024 16:23
unimplemented!("ShiftLeftLogicalImmediate")
// slli: x[rd] = x[rs1] << shamt
let local_rs1 = env.read_register(&rs1);
let shamt = {
Copy link
Member

Choose a reason for hiding this comment

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

Can you add some FIXME reg. this comment please? Can be done later.

Copy link
Member

Choose a reason for hiding this comment

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

Note that a test is ready in master reg. sll, see assembly code. Only lui should be implemented.

let pos = env.alloc_scratch();
unsafe { env.bitmask(&imm, 5, 0, pos) }
};
// parse shamt from imm as 20-24 of instruction and 0-4 wrt to imm
Copy link
Member

Choose a reason for hiding this comment

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

what is this comment for?

Copy link
Member Author

Choose a reason for hiding this comment

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

it was removed, but it was explaining where in the instruction and immediate the shamt was parsed from.

@svv232 svv232 requested a review from dannywillems December 11, 2024 17:38
@dannywillems dannywillems merged commit 7bb2d62 into master Dec 16, 2024
7 of 8 checks passed
@dannywillems dannywillems deleted the sai/shift_left_logical_immediate_riscvi32 branch December 16, 2024 09:59
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.

2 participants