[FIRRTL] Isolate reset inference to a cast rather than connect #4804
Conversation
There was a problem hiding this comment.
Generally LGTM!
Added some comments. Should this support aggregates / references (which are kinda like aggregates in a type sense)?
Can't speak for this direction as brain is a bit full of references, but this makes sense to me re:approach, and breaking out RTP probably needs to happen, thanks!
Should this be added to FIRRTLVisitor ?
| Cast between reset types. This is used to enable strictconnects early in | ||
| the pipeline by isolating all uninferred reset connections to a single op. | ||
| }]; | ||
| let arguments = (ins AnyResetType:$input); |
There was a problem hiding this comment.
Could this instead cast between aggregate types that contain resets? That would help to use this in front of connects before LowerTypes, and similarly with references / ref-define.
There was a problem hiding this comment.
I'm leaning towards splitting connect up instead.
| @@ -88,6 +89,12 @@ void circt::firrtl::emitConnect(ImplicitLocOpBuilder &builder, Value dst, | |||
| return; | |||
| } | |||
|
|
|||
| if ((dstType.hasUninferredReset() || srcType.hasUninferredReset()) && | |||
| dstType != srcType) { | |||
| src = builder.create<UninferredResetCastOp>(dstType, src); | |||
There was a problem hiding this comment.
(if UninferredResetCastOp supports aggregates, this could be hoisted above the checks for connecting bundles/vectors)
| ; CHECK: %3 = firrtl.subfield %b[b] : !firrtl.bundle<a: reset, b: reset> | ||
| ; CHECK: %4 = firrtl.subfield %a[b] : !firrtl.bundle<a: uint<1>, b: asyncreset> | ||
| ; CHECK: %[[r4:.*]] = firrtl.resetCast %4 | ||
| ; CHECK: firrtl.strictconnect %3, %[[r4]] : !firrtl.reset |
There was a problem hiding this comment.
Are there any dedicated tests / errors to check for this new operation?
|
(micro-nit: commit/PR title: s/Issolate/Isolate/) |
darthscsi
changed the title
Add a resetCast operation. This lets more connects be strictconnects as more typeconversion can be expressed explicitly.