[FIRRTL] Limited invalid propagation. #7074
Conversation
Now that invalid is better defined, we can start propagating it when we can preserver semantics. This means not breaking the one-op-is-one-consistent-value (e.g. not duplicating). The only bit mutating op which is touched by this change is not, which is a pure all-bits inversion, which should be safe (tm).
lib/Dialect/FIRRTL/FIRRTLFolds.cpp
Outdated
| // can you propagate through binary ops. | ||
| if (op->hasOneUse() && | ||
| isa<BitsPrimOp, HeadPrimOp, ShrPrimOp, TailPrimOp, SubfieldOp, SubindexOp, | ||
| AsSIntPrimOp, AsUIntPrimOp, NotPrimOp>(*op->user_begin())) { |
There was a problem hiding this comment.
There are a few more unwary ops that can be included: cvt, pad, asClock, and the reduction ops.
There was a problem hiding this comment.
Pad adds bits, so it isn't safe, cvt interprets the bits and can add bits, so likewise. asClock is probably safe.
There was a problem hiding this comment.
But reductions are a good call.
There was a problem hiding this comment.
oddly, a reduction of a zero-width value is NOT invalid, even though all other widths are.
There was a problem hiding this comment.
This is legal per the spec. Can you check the impact on a real design? 🧐
I'm also starting to question if CSE involving invalid value ops is going to make much of this rare to canonicalize.
Re: rarity: do we propagate invalid through single connects with your new wire to node conversion? It's pretty difficult to create an invalid in Chisel as it has to be of a component. Practically, this usually shows up on only ports or wires and both of these are funneling invalid through a connect.
|
Doesn't seem to have any effect on at least one large design. |
Now that invalid is better defined, we can start propagating it when we can preserver semantics. This means not breaking the one-op-is-one-consistent-value (e.g. not duplicating). The only bit mutating op which is touched by this change is not, which is a pure all-bits inversion, which should be safe (tm).