chipsalliance / chipsalliance/rocket-chip
ALU shiftReverse includes FN_ROR unconditionally, leaving residual logic when useZbb=false
- 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
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