Fix: RV32 SLL/SRL/SRA mask shift amount

[mrLSD]

Shift counts >= 32 gave wrong results; add edge-case tests for masking

Summary by CodeRabbit

• Bug Fixes

  • Corrected RV32 register-based shift instructions so large shift amounts are handled correctly, matching expected 32-bit semantics.

• Tests

  • Expanded test coverage for shift instructions with additional edge cases to validate behavior when shift amounts exceed 31 (including logical and arithmetic shifts).

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: c2a303fe-8f14-4e3c-825d-df21f525bd0e

📥 Commits

Reviewing files that changed from the base of the PR and between f9dfe06 and 5afed36.

📒 Files selected for processing (1)

• ExecuteI.fs

🚧 Files skipped from review as they are similar to previous changes (1)

• ExecuteI.fs

---

📝 Walkthrough

Walkthrough

This pull request changes RV32 register-shift semantics to mask the shift amount to rs2[4:0] (0x1f) for SLL, SRL, and SRA; implementations perform shifts on 32-bit-cast operands and return int64 results. Tests for SLL/SRL/SRA are expanded with edge-case shift amounts above 31.

Changes

RV32 Shift Instruction Masking

Layer / File(s)	Summary
Shift instruction semantic fixes ExecuteI.fs	execSLL, execSRL, and execSRA RV32 branches now compute shamt = rs2 & 0x1f, perform the shift on 32-bit-cast operands (using unsigned cast for SRL, signed for SRA), and cast the 32-bit result back to int64.
Shift masking test coverage Tests/rv32i/alu.fs	Added [<InlineData>] cases for SLL, SRL, and SRA covering large shift amounts (e.g., 32–63) to assert masking to 5 bits and correct zero-fill vs sign-fill behavior.

Estimated Code Review Effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

I hop through bits with whiskered care,
Five-bit shamt trimmed from rs2's lair.
Left, logical, arithmetic—each shift aligned,
Tests chase the edges, none left behind. 🐇✨

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Fix: RV32 SLL/SRL/SRA mask shift amount' clearly and specifically summarizes the main change: fixing the shift amount masking for RV32 shift instructions.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/rv32-shift-mask

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (2)

ExecuteI.fs (2)

302-304: ⚡ Quick win

Update or remove misplaced comment block.

This comment describes shift instruction behavior but appears inside the execSLT function (Set Less Than), not inside a shift function. It should either be moved to the appropriate shift instruction or removed from this location.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ExecuteI.fs` around lines 302 - 304, The comment about SLL, SRL, and SRA
(logical left/right and arithmetic right shifts using low 6 bits of rs2 in
RV64I) is misplaced inside execSLT; move that comment to the shift-instruction
handler(s) (e.g., the functions handling SLL, SRL, SRA) or remove it here so
execSLT contains only relevant commentary about set-less-than semantics; ensure
the relocated comment accompanies the shift implementation (references: execSLT,
SLL, SRL, SRA, rs1, rs2, RV64I).

---

329-331: ⚡ Quick win

Update comments to document RV32I masking behavior.

These comments currently state "In RV64I, only the low 6 bits of rs2 are considered for the shift amount" but omit the RV32I behavior. Now that the implementation correctly masks to the low 5 bits for RV32I, the comments should reflect both modes.

📝 Suggested comment update

-// SLL, SRL, and SRA perform logical left, logical right, and arithmetic right shifts on the value in register
-// rs1 by the shift amount held in register rs2. In RV64I, only the low 6 bits of rs2 are considered for the
-// shift amount.
+// SLL, SRL, and SRA perform logical left, logical right, and arithmetic right shifts on the value in register
+// rs1 by the shift amount held in register rs2. In RV32I, only the low 5 bits of rs2 are considered for the
+// shift amount. In RV64I, only the low 6 bits of rs2 are considered for the shift amount.

Also applies to: 346-348

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ExecuteI.fs` around lines 329 - 331, Update the block comments that describe
SLL, SRL, and SRA to document both RV64I and RV32I masking rules: state that in
RV64I only the low 6 bits of rs2 are used as the shift amount and in RV32I only
the low 5 bits are used; apply this change to the comment blocks around the
SLL/SRL/SRA descriptions (the ExecuteI implementation comments) and also update
the corresponding comment at the other occurrence noted (the second block around
the same SLL/SRL/SRA descriptions).

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@ExecuteI.fs`:
- Around line 302-304: The comment about SLL, SRL, and SRA (logical left/right
and arithmetic right shifts using low 6 bits of rs2 in RV64I) is misplaced
inside execSLT; move that comment to the shift-instruction handler(s) (e.g., the
functions handling SLL, SRL, SRA) or remove it here so execSLT contains only
relevant commentary about set-less-than semantics; ensure the relocated comment
accompanies the shift implementation (references: execSLT, SLL, SRL, SRA, rs1,
rs2, RV64I).
- Around line 329-331: Update the block comments that describe SLL, SRL, and SRA
to document both RV64I and RV32I masking rules: state that in RV64I only the low
6 bits of rs2 are used as the shift amount and in RV32I only the low 5 bits are
used; apply this change to the comment blocks around the SLL/SRL/SRA
descriptions (the ExecuteI implementation comments) and also update the
corresponding comment at the other occurrence noted (the second block around the
same SLL/SRL/SRA descriptions).

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 66a9dc1b-4fc5-4634-9328-94d1a7b952b6

📥 Commits

Reviewing files that changed from the base of the PR and between 97097d1 and f9dfe06.

📒 Files selected for processing (2)

• ExecuteI.fs
• Tests/rv32i/alu.fs