[Seq] Canonicalize firreg with preset 0 #7350
Conversation
lib/Dialect/Seq/SeqOps.cpp
Outdated
| if (!presetAttr.getValue().isZero()) | ||
| return {}; |
There was a problem hiding this comment.
I think we can replace it with presetAttr even when preset is not zero?
There was a problem hiding this comment.
You are right, addressed it.
| if (constOp.getValue().isOne()) | ||
| return getResetValue(); | ||
| // value. Works only if no preset. | ||
| if (!presetAttr) |
There was a problem hiding this comment.
This should also work fine if the preset value equals the reset value, right?
There was a problem hiding this comment.
This is actually checking if the reset signal is a constant constant high, then replace the register with the reset value. The reset value need not even be a constant. But if the reset is also a constant and its value matches with the preset, then yes we can replace it.
| if (auto reset = getReset()) | ||
| if (auto constOp = reset.getDefiningOp<hw::ConstantOp>()) | ||
| if (constOp.getValue().isOne()) | ||
| return getResetValue(); |
There was a problem hiding this comment.
Not directly related but I feel this canonicalization is not correct either... I think reset value needs to constant
There was a problem hiding this comment.
okay, seemed fine to me, according to the comments in the code. But lets investigate this further, if its legal or not.
Canonicalize
FirRegwith a constant 0 preset. The pattern was earlier ignoring registers with any preset.This builds on #7289