Can we add checks for invalid configurations? Is `useBitManipCrypto && !useBitManip` legal?

Shouldn't there be a `_` before `$multiLetterString`?

Try ``` val multiLetterExt = (Option.when(tileParams.core.useBitManip)(Seq("Zba", "Zbb", "Zbc")) ++ Option.when(tileParams.core.hasBitManipCrypto)(Seq("Zbkb", "Zbkc", "Zbkx")) ++ ...).flatten val multiLetterString = multiLetterExt.mkString("_") ```

I think this can just be `class ALU(implicit p: Parameters) extends AbstractALU(ALUFN())(p) {

Would `class MultiplierReq(dataBits: Int, tagBits: Int, alufn: ALUFN = ALUFN())` work?

I don't want to make this depend on `HasRocketCoreParameters`.

Instead of deleting this, can you comment out a line, and add that the current spec does not define what sub-extensions constitute the 'B' misa bit?

Can these be done in `ABLUFN.isSub`? etc.

See comment on `abstract class AbstractALU`

Can we make this: ``` class ALUFN ... class ABLUFN extends ALUFN ``` Get rid of the companion object.

Can you make this `abstract class AbstractALU(implicit p: Parameters) extends CoreModule()(p)`?

Can you make these throw an exception if they are accessed? If `ALU` is selected instead of `ABLU`, then no code should try to access `ALU.FN_ADDUW` right?

Shouldn't `bitmanip` be reflected somehow in the dts? I don't know what the exact correct isa string composiiton should be, but it should be implemented here.

Does anything else in `BaseTile/BaseCore` use `useBitManipCrypto/useCryptoNIST/useCryptoSM`? maybe those should be rocket-core only params

Why would user want `useBitManip && !useABLU`. Can we make `useABLU` derived?

Why?

`id_ctrl.alu_fn(ZKN.FN_Len-1,0) === ZKN.FN_AES_KS1` needs additional comparing logic, which should be better to wire a `isAESKS1` signal out form the decoder. you can add a TODO here.

Actually this file is under the Apache license(not SiFive), although SiFive is Apache as well. ```suggestion // SPDX-License-Identifier: Apache-2.0 ```

Actually this file is under the Apache license(not SiFive), although SiFive is Apache as well. ```suggestion // SPDX-License-Identifier: Apache-2.0 ```

Actually this file is under the Apache license(not SiFive), although SiFive is Apache as well. ```suggestion // SPDX-License-Identifier: Apache-2.0 ```

Actually this file is under the Apache license(not SiFive), although SiFive is Apache as well. ```suggestion // SPDX-License-Identifier: Apache-2.0 ```

For a better PPA result. I doubt these MicroOp need more careful designing, but for now(functionally work), it LGTM.

since chipsalliance/chisel3#2518 merged, you should split these lines into a separate file, add note saying: use `chisel3.stdlib.BarrelShifter` after bumping to Chisel 3.6

use `AbstractAlu` here.

OH

OH

I'm not sure the ROM compiler can provide a good PPA here. Need physical implementation.

1-bit control signal.

bad design :(

Put it in ALU.

I think using OH here should provide better circuit.

bad design :(

These code should be auto generated via https://github.com/riscv/riscv-opcodes/blob/master/Makefile

`MircoOp` should redesign.

These code should be auto generated via https://github.com/riscv/riscv-opcodes/blob/master/Makefile

barrel should be upstreamed to Chisel.

These `MicroOp` should be redesigned: firstly design the data path, then figure out all enable signal of registers and select signal of muxes. At last, design the `MicroOp` based on those signals.

These code should be auto generated via https://github.com/riscv/riscv-opcodes/blob/master/Makefile

I think `Mux` here should be implemented via OH