implement sub-word assignments #2545
Conversation
ekiwi
force-pushed
the
sub-word-assign-2
branch
12 times, most recently
from
0644f0a
to
49c7f4d
Compare
ekiwi
force-pushed
the
sub-word-assign-2
branch
from
49c7f4d
to
d382bba
Compare
|
Great that this comes back to Chisel. |
| | x <= y | ||
| |""".stripMargin | ||
| check(src, expected) | ||
| } |
There was a problem hiding this comment.
Might also want a test for subword-assigning an SInt:
circuit test :
module test :
input c : UInt<2>
output x : SInt<2>
x[0] <= c[0]
x[1] <= c[1]
Note that since the type of x[...] is always UInt, it must be connected to a UInt even though x is an SInt.
There was a problem hiding this comment.
Yes. Thanks, I need to add this test (and fix the implementation to support it)
| /** performs special simplifications which are needed in the case of sub-word assignments to avoid false positives | ||
| * in the connection check | ||
| */ | ||
| private def simplifyBits(e: ir.Expression): ir.Expression = e match { |
There was a problem hiding this comment.
I'm not sure if this code already handles this, but I recently fixed a bug in the CIRCT implementation related to SInts so I thought I'd mention it here in case. First the connector must call asSInt if the bit-indexed value being assigned is an SInt.
Then the simplifications need to look through calls to asSInt (i.e., having a simplification that bits(asSInt(x), ..., ...) -> bits(x, ..., ...)).
There was a problem hiding this comment.
I believe subword-assignment for SInt is broken in this PR right now. Thanks for pointing out how I could fix it.
| * CHIRRTL level type inference then converts this into either a `bits(..., ... , ...)`, | ||
| * or [[ir.SubIndex]] depending on the inferred type of the inner expression. | ||
| */ | ||
| case class WSliceNode(expr: ir.Expression, hi: Int, lo: Int) extends ir.Expression { |
There was a problem hiding this comment.
Would it be better to define this as a node in IR.scala so that it can be more easily accessed from the Chisel converter?
There was a problem hiding this comment.
No, this is an internal node used to hack around a short coming in the parser (that it does not do type inference). From Chisel you should know whether you want a bit-extract or a vector access since the types are available in the Chisel data structure.
| | x[0] <= UInt(1) | ||
| | x[1] is invalid | ||
| |""".stripMargin | ||
| // TODO: currently we actually do not perform this optimization since we replace invalid with 0 for sub-word assignments |
There was a problem hiding this comment.
See discussion around this here: llvm/circt#3658 (comment)
I think this is actually super critical that we do this, now that I think about it. My reasoning is that the behavior of a vector of bools needs to be the exact same as the behavior of a uint with bit selects. Otherwise this is defining a fifth invalid interpretation that is inconsistent with something that should be functionally the same.
I grok that just converting to zero is totally legal. However, if the SFC goes this route, it locks in this behavior for Chisel users, they may inadvertently write code which depends on it, and it effectively forces any other FIRRTL compiler to fall in line unless Chisel users are expected to change their code. We hit numerous issues around choices that the SFC made around invalid being or not being zero which would cause simulation failures. 😬 I'm trying to avoid that here.
Contributor Checklist
Type of Improvement
API Impact
Backend Code Generation Impact
Desired Merge Strategy
Release Notes
Reviewer Checklist (only modified by reviewer)
Please Merge?