[rtl] Switch to ratified bitmanip extensions by thommythomaso · Pull Request #2459 · lowRISC/ibex

thommythomaso · 2026-07-13T12:51:40Z

This PR switches the RTL of Ibex from our currently unratified bitmanip extension to the officially ratified extensions.

An overview of the changes can be found here.

This PR only introduces the RTL changes and ensures the CI tests pass. Certain scalar crypto extensions are not yet fully verified; see #2468.

marnovandermaas

Initial comments from me. Do you think it is worth squashing the commits that just remove things? I think it may be excessive to have 9 separate commits for removing instructions.

thommythomaso · 2026-07-15T14:55:39Z

Initial comments from me. Do you think it is worth squashing the commits that just remove things? I think it may be excessive to have 9 separate commits for removing instructions.

I have them as individual commits to help people review the changes. I would have squashed all into one after the review process. What do you think?

SamuelRiedel

Thanks for cleaning up the bitmanip @thommythomaso. I did a quite thorough review, but most of it should be easy to fix.

The only thing that is left is that we should update RISC-V DV to also support the latest ratified extensions. I saw that the ratified B extensions are supported already, but for the scalar crypto ones, we currently simply enable the Zbp still, right? I think this is good enough for now, but we should create an issue documenting the current state, like which instructions are emitted by riscv-dv and which aren't and what we would ideally want to then clean up the DV in a second step at least.

This PR removes the unratified bitmanip extensions from Ibex and switches to the ratified set. We remove `Xzbe0p93`, `Xzbf0p93`, `Xzbr0p93`, `Xzbt0p93`, and `Xzbe0p93`, replacing them with `Zbkb` and `Zbkx`. Signed-off-by: Thomas Benz <tbenz@lowrisc.org>

vogelpi

Thanks @thommythomaso for taking this effort on in the first place and for the PR. This is nice work!

Apart from a couple of nits, I think we should do the following things before merging this PR:

We should consider implementing ror/rol in the shifter rather than making it a multicycle instruction. Because these instructions were never a driver for introducing multi-cycle instructions in the ALU, but once the support was there, it was good to re-use the infrastructure to avoid making the shifter more complex. Now, it would be possible to have everything single cycle and I believe moving the complexity to the shifter would not increase the critical path but reduce complexity and area.
There are quite a few single-line (plus default) unique_case statements in the decoder now. While it's easier to review the changes as is. We should rewrite these statements IMO to increase readability.

In general, I wondering how confident we are that when adding DV support for the scalar crypto, we don't find out that stuff is missing or not properly implemented? Have @SamuelRiedel and @marnovandermaas done a thorough enough review such that we are reasonable confident? On my end, I mostly focused on ensuring we don't drop an instruction which is not meant to be dropped (according to your sheet @thommythomaso ).

vogelpi · 2026-08-10T11:44:51Z

+   * - **B**: Standard Extension for Bit-Manipulation Instructions (Zba, Zbb, Zbc, Zbs)
+     - 1.0.0
+     - optional


Please note that B just comprises Zba, Zbb and Zbs. The collection comprising Zba, Zbb, Zbc and Zbs is called RISC-V bit manipulation extension v.1.0.0 (see https://www.ece.lsu.edu/ee4720/doc/riscv-bitmanip-1.0.0.pdf, it's the same doc you referenced above but this doesn't reference "B" anymore).

So, here we should probably say:

B + Zbc because that's what we use our ISA string

or then Zba, Zbb, Zbc, Zbs as above

vogelpi · 2026-08-10T12:02:01Z

  always_comb begin
-    if (bfp_op) begin
-      shift_amt[4:0] = bfp_off;  // length field of bfp control word
-    end else begin
-      shift_amt[4:0] = instr_first_cycle_i ?
-          (operand_b_i[5] && shift_funnel ? shift_amt_compl[4:0] : operand_b_i[4:0]) :
-          (operand_b_i[5] && shift_funnel ? operand_b_i[4:0] : shift_amt_compl[4:0]);
-    end
+    shift_amt[4:0] = instr_first_cycle_i ? operand_b_i[4:0] : shift_amt_compl[4:0];
  end


You now have an always_comb with a single ternary statement in it. Can you please just convert this to an assign?

vogelpi · 2026-08-10T12:10:04Z

    end else begin
      unique case (1'b1)
-        bfp_op:       shift_operand = bfp_mask_rev;
        shift_sbmode: shift_operand = 32'h8000_0000;
        default:      shift_operand = shift_left ? operand_a_rev : operand_a_i;


You now have quite a few unique case (1'b1) statements with a single case plus the default in here. I think an if/else would make more sense but I also appreciate the diff now being smaller meaning the review is simpler.

vogelpi · 2026-08-10T12:11:42Z

+    ////////////////////////////////
+    // rev8 / brev8 / orc.b        //
+    ////////////////////////////////


As you anyway change this, is there a reason for not minimizing the width of the header? i.e.,

////////////////////////// // rev8 / brev8 / orc.b // //////////////////////////

vogelpi · 2026-08-10T12:29:05Z

+    // Shift Rotations
+    // operand_a_i is tied to rs1, operand_b_i is tied to rs2.


The shift rotations are now the only multi-cycle instructions of the ALU and those are the most simple instructions of the original set of multi-cycle instructions. IIRC, we decided to make them multi-cycle as the register is now there anyway and it was better to use it compared to also making the shifter wider.

But now, we only keep the register and the control (+ verification complexity) to implement an instruction which could probably simply be implemented with a slightly different shifter. Have you considered this option?

vogelpi · 2026-08-10T12:36:47Z

              5'b0_0101,                                                              // bseti
-              5'b0_1101: illegal_insn = (RV32B != RV32BNone) ? 1'b0 : 1'b1;           // binvi
+              5'b0_1101: illegal_insn = (instr[26:25] == 2'b00 && RV32B != RV32BNone) ?
+                                         1'b0 : 1'b1;                                  // binvi


Nit: the // binvi comment should be moved left by one space ;-)

vogelpi · 2026-08-10T12:38:30Z

                5'b0_1100,                                                             // rori
-                5'b0_1001: illegal_insn = (RV32B != RV32BNone) ? 1'b0 : 1'b1;          // bexti
+                5'b0_1001: illegal_insn = (instr[26:25] == 2'b00 && RV32B != RV32BNone) ?
+                                           1'b0 : 1'b1;                                 // bexti


Nit: move comment // bexti left by 1 space.

vogelpi · 2026-08-10T12:40:11Z

-                    illegal_insn = 1'b1;
-                  end
+                  // orc.b (Zbb): gorci restricted to shamt 0x07
+                  illegal_insn = (instr[25:20] == 6'b000111 && RV32B != RV32BNone) ?


Nit: 6'00_0111 would be more readable.

SamuelRiedel · 2026-08-11T10:30:37Z

Thanks @thommythomaso for taking this effort on in the first place and for the PR. This is nice work!

Apart from a couple of nits, I think we should do the following things before merging this PR:

We should consider implementing ror/rol in the shifter rather than making it a multicycle instruction. Because these instructions were never a driver for introducing multi-cycle instructions in the ALU, but once the support was there, it was good to re-use the infrastructure to avoid making the shifter more complex. Now, it would be possible to have everything single cycle and I believe moving the complexity to the shifter would not increase the critical path but reduce complexity and area.

There are quite a few single-line (plus default) unique_case statements in the decoder now. While it's easier to review the changes as is. We should rewrite these statements IMO to increase readability.

In general, I wondering how confident we are that when adding DV support for the scalar crypto, we don't find out that stuff is missing or not properly implemented? Have @SamuelRiedel and @marnovandermaas done a thorough enough review such that we are reasonable confident? On my end, I mostly focused on ensuring we don't drop an instruction which is not meant to be dropped (according to your sheet @thommythomaso ).

@vogelpi I reviewed the hardware thoroughly, including the scalar crypto extensions, and I am confident we aren't missing any instructions. As noted though, the DV still needs cleanup. While I'd prefer a clean DV run to ensure all corner cases are covered, this PR doesn't add any new instructions/logic and the old DV still runs. Therefore, IMO we match the current D1 state, can merge the RTL, and address the DV in the next step.

thommythomaso self-assigned this Jul 13, 2026

thommythomaso requested a review from SamuelRiedel July 13, 2026 12:51

thommythomaso force-pushed the tbenz/bitmanip branch 4 times, most recently from afac420 to 6062faa Compare July 15, 2026 12:38

thommythomaso mentioned this pull request Jul 15, 2026

[ibex, rtl] Move to ratified bitmanip extensions lowRISC/opentitan#30502

Draft

marnovandermaas reviewed Jul 15, 2026

View reviewed changes

Comment thread rtl/ibex_decoder.sv

Comment thread rtl/ibex_decoder.sv Outdated

Comment thread doc/01_overview/compliance.rst

thommythomaso marked this pull request as ready for review July 16, 2026 06:53

SamuelRiedel reviewed Jul 20, 2026

View reviewed changes

thommythomaso force-pushed the tbenz/bitmanip branch from e3c3ce9 to 753e7ac Compare August 6, 2026 13:19

thommythomaso force-pushed the tbenz/bitmanip branch from 753e7ac to 5a51b6c Compare August 7, 2026 05:57

andreaskurth requested a review from vogelpi August 10, 2026 11:16

vogelpi reviewed Aug 10, 2026

View reviewed changes

		// Shift Rotations
		// operand_a_i is tied to rs1, operand_b_i is tied to rs2.

Uh oh!

Conversation

thommythomaso commented Jul 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marnovandermaas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thommythomaso commented Jul 15, 2026

Uh oh!

SamuelRiedel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vogelpi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SamuelRiedel commented Aug 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

thommythomaso commented Jul 13, 2026 •

edited

Loading