[FIRRTL] Add bit-index support (subword assignment) #3525
Conversation
There was a problem hiding this comment.
Welcome! Please see comments in the firrtl spec PR too.
In general, this op seems purely redundant and more limited than BitsPrimOp. It's not obvious why we need a new op to accomplish the same thing, redundancy is bad in an IR. Further, from a lowering perspective (and generally), it's not clear why this isn't treated like a write to a subindex or subfield.
Some open questions, also noted in the firrtl spec PR:
- Why? we have bits(), this is just a limited form of that?
- What if the input has inferred widths?
- How are we suppose to interpret last-connect semantics with sub-word updates?
- How is combinatorial loop detection expected to work? Must it be bit-sensitive?
| value, resultValue, | ||
| "firrtl::getBitindexElementType($_self)">; | ||
|
|
||
| def BitindexOp : FIRRTLExprOp<"bitindex", [ |
There was a problem hiding this comment.
This seems redundant with BitsPrimOp.
lib/Dialect/FIRRTL/FIRRTLOps.cpp
Outdated
| @@ -2489,6 +2490,35 @@ FIRRTLType SubindexOp::inferReturnType(ValueRange operands, | |||
| return {}; | |||
| } | |||
|
|
|||
| bool BitindexOp::isBitIndex(ValueRange operands, ArrayRef<NamedAttribute> attrs, | |||
| Optional<Location> loc) { | |||
| auto inType = operands[0].getType(); | |||
There was a problem hiding this comment.
return input.getType().isa<IntType>();
Which makes having a whole funciton questionable.
lib/Dialect/FIRRTL/FIRRTLOps.cpp
Outdated
| @@ -2489,6 +2490,35 @@ FIRRTLType SubindexOp::inferReturnType(ValueRange operands, | |||
| return {}; | |||
| } | |||
|
|
|||
| bool BitindexOp::isBitIndex(ValueRange operands, ArrayRef<NamedAttribute> attrs, | |||
| Optional<Location> loc) { | |||
| return UIntType::get(inType.getContext(), 1); | ||
| } | ||
| if (loc) | ||
| mlir::emitError(*loc, "out of range index '") |
There was a problem hiding this comment.
please do verification with a trait or verifier, not in the return type inference.
| } | ||
|
|
||
| if (loc) | ||
| mlir::emitError(*loc, "bitindex requires UInt or SInt"); |
There was a problem hiding this comment.
This should be enforced by the op traits.
| @@ -625,6 +625,13 @@ Type firrtl::getVectorElementType(Type array) { | |||
| return vectorType.getElementType(); | |||
| } | |||
|
|
|||
| Type firrtl::getBitindexElementType(Type array) { | |||
| if (array.isa<IntType>()) { | |||
There was a problem hiding this comment.
This is excessively defensive for something which should already be guaranteed.
| @@ -1824,10 +1824,22 @@ ParseResult FIRStmtParser::parsePostFixIntSubscript(Value &result) { | |||
| // builder (https://llvm.discourse.group/t/3504). | |||
| NamedAttribute attrs = {getConstants().indexIdentifier, | |||
| builder.getI32IntegerAttr(indexNo)}; | |||
| auto resultType = SubindexOp::inferReturnType({result}, attrs, {}); | |||
| bool bitindex = BitindexOp::isBitIndex({result}, attrs, {}); | |||
There was a problem hiding this comment.
This hints at poor firrtl syntax. Since you are also proposing the firrtl syntax, there is no reason for this to be ambiguous.
| @@ -1843,6 +1855,12 @@ ParseResult FIRStmtParser::parsePostFixIntSubscript(Value &result) { | |||
| locationProcessor.setLoc(loc); | |||
| OpBuilder::InsertionGuard guard(builder); | |||
| builder.setInsertionPointAfterValue(result); | |||
| if (bitindex) { | |||
| auto op = builder.create<BitindexOp>(resultType, result, attrs); | |||
There was a problem hiding this comment.
perhaps just put the build in the conditional, since the rest of this matches the old code.
| // | ||
| //===----------------------------------------------------------------------===// | ||
| // | ||
| // This file defines the LowerBitindex pass. |
There was a problem hiding this comment.
Please explain what the pass does at a high level.
| // | ||
| // This file defines the LowerBitindex pass. | ||
| // | ||
| //===----------------------------------------------------------------------===// |
There was a problem hiding this comment.
It's not clear why there is a lowering pass for this. This is directly expressible in SV with IndexedPartSelectInOutOp.
|
Closing this in favor of #3658. |
This PR adds support for the bit-index operator in FIRRTL. See chipsalliance/firrtl-spec#26 for the proposal and a description of the lowering algorithm. This PR adds support via a lowering pass in the FIRRTL dialect called
LowerBitindex. This is my first time using/contributing to this codebase so there are probably many things in my code that could be done better. Please let me know what to change. Thanks!