chipsalliance / chipsalliance/rocket-chip

ALU shiftReverse includes FN_ROR unconditionally, leaving residual logic when useZbb=false

Open Beginner friendly
#3,784 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Scala
Stars
3.9k
Forks
1.3k
Avg merge
5d 13m
Merged PRs (30d)
1

Description

## Title: ALU `shiftReverse` includes `FN_ROR` unconditionally, leaving residual logic when `useZbb=false`

### Summary

In the default `RocketCoreParams` configuration (`useZbb = false`), the ALU function code `FN_ROR` (18, `5'h12`) participates in the shift-input mux via `shiftReverse`, but has no corresponding output mux arm. The result is a small amount of dead wiring in the synthesized netlist.

This is not a correctness issue — the decoder never emits `FN_ROR` when Zbb is disabled, and the ALU output is identically zero for this fn code. We are reporting this as an observation from formal verification research, in case the team considers it worth addressing.

### Details

`shiftReverse` in the `ALU` companion object unconditionally includes `FN_ROR`:

```scala
// ALU.scala:64 (companion object — no parameter guard)
def shiftReverse(cmd: UInt) = !cmd.isOneOf(FN_SR, FN_SRA, FN_ROR, FN_BEXT)
```

This generates an `io_fn == 5'h12` comparator in the `shin` mux of the elaborated RTL (`RocketALU.sv`), regardless of configuration.

However, the output `MuxLookup` only includes `FN_ROR -> rotout` inside the `if (coreParams.useZbb)` guard:

```scala
// ALU.scala:163-174
val out = MuxLookup(io.fn, shift_logic_cond)(Seq(
FN_ADD -> io.adder_out,
FN_SUB -> io.adder_out
) ++ (if (coreParams.useZbb) Seq(
...
FN_ROL -> rotout,
FN_ROR -> rotout, // only present when useZbb=true
) else Nil))
```

And the decoder (`ZbbDecode` in `IDecode.scala:466`) is similarly gated:

```scala
// RocketCore.scala:243
(if (coreParams.useZbb) Seq(new ZbbDecode, ...) else Nil)
```

So fn=18 is dead from both ends: the decoder never produces it, and the output mux never selects it.

### Verification

We confirmed this with Z3 on the Yosys-generated SMT2 model of `RocketALU.sv` (from commit `885dd59`, CIRCT firtool-1.62.1, default params):

- Asserting `io_out != 0` with `io_fn = 5'b10010` returns **UNSAT** for both `dw=1` (DW_64) and `dw=0` (DW_32), confirming the output is identically zero over all 2^129 input combinations.

An exhaustive scan of all 32 fn codes shows fn=0x12 is the **only** dead code that leaves residual hardware in the shin mux. All other dead fn codes (e.g., `FN_UNARY`, `FN_ROL`, `FN_MAX`/`FN_MIN`) are cleanly eliminated because they have no unconditional references.

### Impact

- **Correctness:** None. The ALU produces zero, and no decode path triggers it.
- **Area:** Minimal. One 5-bit comparator + one OR gate in the shin mux select. Likely optimized by downstream synthesis tools.
- **Timing:** Negligible.

### Possible fix

Move the `FN_ROR` reference in `shiftReverse` behind the same parameter guard:

```scala
// Option A: inline in class ALU (parameter-aware)
val shin = Mux(!io.fn.isOneOf(
Seq(FN_SR, FN_SRA) ++
(if (coreParams.useZbs) Seq(FN_BEXT) else Nil) ++
(if (coreParams.useZbb) Seq(FN_ROR) else Nil)
: _*), Reverse(shin_r), shin_r)
```

Or keep the current behavior — it is functionally correct and the overhead is negligible.

### Environment

- rocket-chip commit: `885dd59` (chipsalliance/rocket-chip, Feb 25 2026)
- CIRCT: firtool-1.62.1
- Configuration: default `RocketCoreParams()` (`useZbb=false`, `useZbs=false`)
- Verification: Yosys 0.38+ → SMT2 → Z3 4.12+

Contributor guide

Open the contributing guide

Research direction

Start in ALU.scala at the ALU companion object's shiftReverse definition and compare its FN_ROR handling with the useZbb-guarded output mux and decoder references described here. Validate the default RocketCoreParams configuration by regenerating RocketALU.sv and checking that the unnecessary FN_ROR comparator is absent while behavior remains unchanged.

Written by the indexing model from the issue text.

Assessment

Tech stack
scala
Domain
embedded-iot
Issue type
Refactor
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
62/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.