[FIRRTL][LowerToHW] Verification of async reset value is not correct
Nobody has claimed this yet.
- Dominant language
- C++
- Stars
- 2.2k
- Forks
- 524
- Avg merge
- 3d 2h
- Merged PRs (30d)
- 46
Description
`test/Conversion/FIRRTLToHW/lower-to-hw.mlir` is currently missing `-verify-diagnostics` flag and producing the following unexpected errors overlooked by tests.
```
/home/uenoku/dev/circt/test/Conversion/FIRRTLToHW/lower-to-hw.mlir:1144:11: error: register with async reset requires constant reset value
%r0 = firrtl.regreset %clock, %arst, %constWire : !firrtl.asyncreset, !firrtl.uint<42>, !firrtl.uint<42>
^
/home/uenoku/dev/circt/test/Conversion/FIRRTLToHW/lower-to-hw.mlir:1144:11: note: see current operation: %10 = "firrtl.regreset"(%2, %3, %8) {annotations = [], name = "r0"} : (!firrtl.clock, !firrtl.asyncreset, !firrtl.uint<42>) -> !firrtl.uint<42>
/home/uenoku/dev/circt/test/Conversion/FIRRTLToHW/lower-to-hw.mlir:1140:18: note: reset value defined here:
%constWire = firrtl.wire : !firrtl.uint<42>
^
/home/uenoku/dev/circt/test/Conversion/FIRRTLToHW/lower-to-hw.mlir:1144:11: error: 'firrtl.regreset' op LowerToHW couldn't handle this operation
%r0 = firrtl.regreset %clock, %arst, %constWire : !firrtl.asyncreset, !firrtl.uint<42>, !firrtl.uint<42>
^
/home/uenoku/dev/circt/test/Conversion/FIRRTLToHW/lower-to-hw.mlir:1144:11: note: see current operation: %10 = "firrtl.regreset"(%2, %3, %8) {annotations = [], name = "r0"} : (!firrtl.clock, !firrtl.asyncreset, !firrtl.uint<42>) -> !firrtl.uint<42>
/home/uenoku/dev/circt/test/Conversion/FIRRTLToHW/lower-to-hw.mlir:1152:11: error: register with async reset requires constant reset value
%r0 = firrtl.regreset %clock, %arst, %constNode : !firrtl.asyncreset, !firrtl.uint<42>, !firrtl.uint<42>
^
/home/uenoku/dev/circt/test/Conversion/FIRRTLToHW/lower-to-hw.mlir:1152:11: note: see current operation: %6 = "firrtl.regreset"(%1, %2, %4) {annotations = [], name = "r0"} : (!firrtl.clock, !firrtl.asyncreset, !firrtl.uint<42>) -> !firrtl.uint<42>
/home/uenoku/dev/circt/test/Conversion/FIRRTLToHW/lower-to-hw.mlir:1150:18: note: reset value defined here:
%constNode = firrtl.node %c1337_ui42 : !firrtl.uint<42>
^
/home/uenoku/dev/circt/test/Conversion/FIRRTLToHW/lower-to-hw.mlir:1152:11: error: 'firrtl.regreset' op LowerToHW couldn't handle this operation
%r0 = firrtl.regreset %clock, %arst, %constNode : !firrtl.asyncreset, !firrtl.uint<42>, !firrtl.uint<42>
^
/home/uenoku/dev/circt/test/Conversion/FIRRTLToHW/lower-to-hw.mlir:1152:11: note: see current operation: %6 = "firrtl.regreset"(%1, %2, %4) {annotations = [], name = "r0"} : (!firrtl.clock, !firrtl.asyncreset, !firrtl.uint<42>) -> !firrtl.uint<42>
```
This verification is introduced in https://github.com/llvm/circt/pull/1444 and it seems `firrtl::isConstant` doesn't work well for constant wire/node. It is necessary to care about the direction of data dependency in the worklist iterations of `firrtl::isConstant`.
Contributor guide
No contributing guide indexed for this repository
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with test/Conversion/FIRRTLToHW/lower-to-hw.mlir and run it with -verify-diagnostics to reproduce the overlooked errors. Then inspect the LowerToHW verification path around firrtl::isConstant; done means constant wire and node reset values are handled correctly while genuinely nonconstant async reset values remain rejected.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- compilers
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 45/100