llvm / llvm/circt

[FIRRTL] ProbeOp + LowerTypes crash on non-passive type

Open
#4,649 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
C++
Stars
2.2k
Forks
524
Avg merge
3d 2h
Merged PRs (30d)
46

Description

LowerTypes crashes with ProbeOp and probably for any unhandled operation that uses non-passive types:

```
$ firtool probe_flips.mlir --mlir-print-ir-before=firrtl-lower-types --mlir-print-ir-after-failure
// -----// IR Dump Before LowerFIRRTLTypes (firrtl-lower-types) //----- //
firrtl.circuit "ProbeFlips" {
firrtl.module @ProbeFlips(in %bundle: !firrtl.bundle, b flip: uint<2>>) {
firrtl.probe @baz, %bundle : !firrtl.bundle, b flip: uint<2>>
%0 = firrtl.subfield %bundle[b] : !firrtl.bundle, b flip: uint<2>>
%c0_ui2 = firrtl.constant 0 : !firrtl.uint<2>
firrtl.strictconnect %0, %c0_ui2 : !firrtl.uint<2>
}
}

probe_flips.mlir:3:5: error: bitwidth cannot be determined for result type '!firrtl.bundle, b flip: uint<2>>'
firrtl.probe @baz, %bundle: !firrtl.bundle, b flip: uint<2>>
^
probe_flips.mlir:3:5: note: see current operation: %1 = "firrtl.bitcast"(%0) : (!firrtl.uint<3>) -> !firrtl.bundle, b flip: uint<2>>
// -----// IR Dump After LowerFIRRTLTypes Failed (firrtl-lower-types) //----- //
"firrtl.circuit"() ({
"firrtl.module"() ({
^bb0(%arg0: !firrtl.uint<1>, %arg1: !firrtl.uint<2>):
%0 = "firrtl.cat"(%arg0, %arg1) : (!firrtl.uint<1>, !firrtl.uint<2>) -> !firrtl.uint<3>
%1 = "firrtl.bitcast"(%0) : (!firrtl.uint<3>) -> !firrtl.bundle, b flip: uint<2>>
"firrtl.probe"(%1) {inner_sym = "baz"} : (!firrtl.bundle, b flip: uint<2>>) -> ()
%2 = "firrtl.constant"() {value = 0 : ui2} : () -> !firrtl.uint<2>
"firrtl.strictconnect"(%arg1, %2) : (!firrtl.uint<2>, !firrtl.uint<2>) -> ()
}) {annotations = [], parameters = [], portAnnotations = [[], []], portDirections = -2 : i2, portLocations = [loc("probe_flips.mlir":2:32), loc("probe_flips.mlir":2:32)], portNames = ["bundle_a", "bundle_b"], portSyms = [], portTypes = [!firrtl.uint<1>, !firrtl.uint<2>], sym_name = "ProbeFlips"} : () -> ()
}) {annotations = [], name = "ProbeFlips"} : () -> ()
```

cc #4648 re:cast to non-passive type (with that PR, error changes to complaining about cast result type not being passive).

Depending what we want ProbeOp for, it can be thought of as naming a set of values (for use as source-flow, in say a bind), in which case converting to passive seems reasonable -- perhaps so much so we change probe to take only passive inputs regardless of aggregates/type-lowering.

Regardless of what we want to do with ProbeOp (which is presently entirely not used from user input), this should be handled with a nicer error if we can't handle it in `LowerTypes`.
(e.g., foreign operations we can't rewrite to passive or expand to the scalar elements of the aggregate).

Contributor guide

No contributing guide indexed for this repository

Research direction

Reproduce the failure with firtool using the probe_flips.mlir input and inspect the LowerTypes pass around ProbeOp handling. Determine whether non-passive ProbeOp inputs should be supported or rejected, then ensure the pass handles the case without crashing and reports the intended outcome clearly.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
compilers
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.