[FIRRTL] Update port insertion/erasure API for instance/instance-choice ops #9093
Conversation
rwy7
force-pushed
the
instance-op-insert-and-erase-ports
branch
from
4eb4dde to
f6962ea
Compare
seldridge
left a comment
seldridge
left a comment
There was a problem hiding this comment.
This looks great. Nits throughout.
| /// Clone this instance with inserted ports. | ||
| [[nodiscard]] InstanceOp cloneWithInsertedPorts( | ||
| ArrayRef<std::pair<unsigned, PortInfo>> insertions); |
There was a problem hiding this comment.
The [[nodiscard]] is a great idea!
There was a problem hiding this comment.
After thinking more about this, I'm now questioning if this makes sense. None of the upstream operations like this, e.g., clone are restricted like this. Also, the couple of times that (void) casts to avoid the unused shows up indicates this may be an anti-pattern.
There was a problem hiding this comment.
I really only wanted to put nodiscard on the functions insertPorts/erasePorts, because it is an easy mistake to think that these functions are updating the op in-place. I don't really think it's necessary on cloneWithInsertedPorts or cloneWithInsertedPortsAndReplaceUses, so I can remove nodiscard from these functions. I'm partial to keeping nodiscard on insertPorts/erasePorts... but now I wonder if we should just remove these two functions instead.
There was a problem hiding this comment.
OK dropped the nodiscard, and dropped insertPorts/erasePorts.
| /// Clone and replace with inserted ports, then drop this instance. | ||
| [[nodiscard]] InstanceOp insertPorts( | ||
| ArrayRef<std::pair<unsigned, PortInfo>> insertions); |
There was a problem hiding this comment.
Nit: it seems slightly inconsistent that as the methods do more, their names get longer, except for insertPorts and erasePorts. It's also weird that the shortest methods do the most work. It may be clearer to either use a long name or to drop the variants that also erase (given that this is easy enough to do in a second function call).
This is just commentary as I'm not sure what makes the most sense here. Also, am I reading this correctly in that erasePorts will now delete the op whereas previously it wouldn't? It seems better to just drop ::erasePorts entirely as it should be fully replaced with cloneWithInsertedPortsAndReplaceUses.
| /// Clone this instance with inserted ports. | ||
| [[nodiscard]] InstanceChoiceOp cloneWithInsertedPorts( | ||
| ArrayRef<std::pair<unsigned, PortInfo>> insertions); | ||
|
|
||
| /// Clones with inserted ports, then replaces uses. | ||
| [[nodiscard]] InstanceChoiceOp cloneWithInsertedPortsAndReplaceUses( | ||
| ArrayRef<std::pair<unsigned, PortInfo>> insertions); | ||
|
|
||
| /// Clone and replace with inserted ports, then drop this instance. | ||
| [[nodiscard]] InstanceChoiceOp | ||
| insertPorts(ArrayRef<std::pair<unsigned, PortInfo>> insertions); | ||
|
|
||
| /// Clone this instance with erased ports. | ||
| [[nodiscard]] InstanceChoiceOp | ||
| cloneWithErasedPorts(const llvm::BitVector &erasures); | ||
|
|
||
| /// Clone this instance with erased ports, then replaces uses. | ||
| [[nodiscard]] InstanceChoiceOp | ||
| cloneWithErasedPortsAndReplaceUses(const llvm::BitVector &erasures); | ||
|
|
||
| /// Clone and replace with erased ports, then drop this instance. | ||
| [[nodiscard]] InstanceChoiceOp erasePorts(const llvm::BitVector &erasures); |
There was a problem hiding this comment.
Great! Would these make sense to raise into the FInstanceLike op interface (which would then necessitate them being implemented for ObjectOp as well)?
There was a problem hiding this comment.
It seems like a good idea, but not something I want to do in this PR.
| if (disabledPorts.any()) { | ||
| OpBuilder builder(instance); | ||
| auto newInstance = instance.erasePorts(builder, disabledPorts); | ||
| instance->erase(); | ||
| instance = newInstance; | ||
| } |
There was a problem hiding this comment.
Nice that this avoids the conditional now.
| size_t erased = 0; | ||
| for (size_t i = 0, e = op1->getNumResults(); i < e; ++i) { |
There was a problem hiding this comment.
Same nit as above: consider moving erased into the initialization of the for loop as it has no users outside the loop.
| for (size_t i = 0, e = op1->getNumResults(); i < e; ++i) { | ||
| auto r1 = op1->getResult(i); | ||
| if (erasures[i]) { | ||
| assert(r1.use_empty() && "removed instance port has uses"); |
There was a problem hiding this comment.
This assertion feels slightly misplaced as it creates an unexpected precondition that isn't related to the correct operation of this method. Probably remove and then reorder the auto r1 to be closer to its (now) sole use or inline the single use if still readable.
There was a problem hiding this comment.
The check was present before, so I decided to preserve it. I agree it is misplaced but I think I would rather have it than not.
| const llvm::BitVector &portIndices) { | ||
| assert(portIndices.size() >= getNumResults() && | ||
| "portIndices is not at least as large as getNumResults()"); | ||
| static void replaceUsesRespectingInsertedPorts( |
There was a problem hiding this comment.
This and the subsequent method are fantastic to have in static functions. 💯
| for (size_t i = 0; i < oldPortCount; ++i) { | ||
| while (inserted < numInsertions) { | ||
| auto &[index, info] = insertions[inserted]; | ||
| if (index > i) | ||
| break; | ||
|
|
||
| auto domains = fixDomainInfoInsertions( | ||
| context, info.domains ? info.domains : empty, indexMap); | ||
| newPortDirections.push_back(info.direction); | ||
| newPortNames.push_back(info.name); | ||
| newPortTypes.push_back(info.type); | ||
| newPortAnnos.push_back(info.annotations.getArrayAttr()); | ||
| newDomainInfo.push_back(domains); | ||
| ++inserted; | ||
| } | ||
|
|
||
| newPortDirections.push_back(getPortDirection(i)); | ||
| newPortNames.push_back(getPortName(i)); | ||
| newPortTypes.push_back(getType(i)); | ||
| newPortAnnos.push_back(getPortAnnotation(i)); | ||
| newDomainInfo.push_back(getDomainInfo()[i]); | ||
| indexMap[i] = i + inserted; | ||
| } |
There was a problem hiding this comment.
Is there a way to share this code with that in the instance choice version?
There was a problem hiding this comment.
I think so, but maybe we can do that in a follow up. I've got all the ducks in a row now so it should be an easy PR.
rwy7
force-pushed
the
instance-op-insert-and-erase-ports
branch
3 times, most recently
from
2980dc3 to
b17b122
Compare
Add missing helpers to the instance choice op, and make some simplifications to the helpers already present on the instance op, to make insertion/erasure of ports more symmetric and similar between instance and instance-choice ops.
rwy7
force-pushed
the
instance-op-insert-and-erase-ports
branch
from
b17b122 to
6000d91
Compare
2 participants
Align and flush out the functionality for port insertion and erasure on instance and instance-choice op.
Overview of new API for Port Insertions:
cloneWithInsertedPorts: create a clone of the op with additional inserted ports. Returns the clone. This API must clone, even if no ports are inserted.cloneWithInsertedPortsAndReplaceUses: replace the uses of this op with a clone created via the above API. This API must clone, even if no ports are inserted.insertPorts: the "do what I mean" API: clone with port insertions, replace uses, and then erase the old instance, but only if there are any ports being inserted.Then for port erasure, we have a parallel set of functions:
cloneWithErasedPortscloneWithErasedPortsAndReplaceUseserasePortsAll
sixfour functions are implemented for both the InstanceOp and InstanceChoiceOp.