[FIRRTL] Allow GC Companion multi-instantiation #8603
Conversation
| builder.create<WireOp>(port.getLoc(), port.getType()); | ||
| port.replaceAllUsesWith(wire.getResult()); | ||
| } | ||
| instance->erase(); |
There was a problem hiding this comment.
I think it's necessary to erase through InstanceGraph.
There was a problem hiding this comment.
Yeah, this isn't keeping the instance graph up to date with its deletions. Same problem later when the module is deleted when in "drop" mode. This code was like this before. I will still fix it here. Good catch.
There was a problem hiding this comment.
Added this in aed1af7. I think that looks right?
| return true; | ||
| } | ||
| // Extract all instances in the design. | ||
| for (auto *i : instancePaths->instanceGraph.lookup(op)->uses()) { |
There was a problem hiding this comment.
Early increment would be necessary
| for (auto *i : instancePaths->instanceGraph.lookup(op)->uses()) { | |
| for (auto *i : make_early_inc_range(..)) { |
There was a problem hiding this comment.
Change in 7c1a1d7.
I think this, as written, should be fine as the instance graph was never updated. However, now that the instance graph is updated, this can definitely be a problem.
| if (!instanceInfo->allInstancesUnderEffectiveDut( | ||
| i->getParent()->getModule())) |
There was a problem hiding this comment.
I guess if there is an instance under effective dut and one is not under dut, companion module will be kept under dut. Does that happen practically?
There was a problem hiding this comment.
No, that would be very rare. Practically, this should never happen because of the dedup group annotations that Chisel emits. It is expected that the test and the design are kept separate to avoid behavior like this. The behavior here is more a stop gap of needing to do something sane if this ever occurs.
Clarifying the behavior: a parent instantiates the companion. We will not extract the companion if the parent is never instantiated in the effective design. Any instantiation in the effective design will force us to extract this.
There is potentially a better (though surprising) behavior of doing module duplication in this situation. However, we've generally tried to avoid this. The surprising/unpredictable nature has been viewed as unacceptable.
In preparation to remove the exactly-one-instance check of each Grand Central Companion module, inline the function doing this check and get this in the right shape to switch it over to allow for multiple instantiation. Signed-off-by: Schuyler Eldridge <[email protected]>
Change the Grand Central View lowering pass to now allow for Grand Central Companions to be multiply instantiated. This should have technically been possible for a long time [[1]] as long as there was a guarantee that there was never going to be a leaf that was _outside_ the companion. This is a pretty simple change to just loop over all instances and extract (add a `lowerToBind` attribute) for the ones which are in the design. The other, subtle blocker is that without the `InstanceInfo`, doing the check of "in the design" was tedious. Now this is pretty straightforward. [1]: 2d3a040 Signed-off-by: Schuyler Eldridge <[email protected]>
Fix a bug in the Grand Central pass where it was deleting instances and modules, but not keeping the instance graph up to date. Signed-off-by: Schuyler Eldridge <[email protected]>
seldridge
force-pushed
the
dev/seldridge/firrtl-allow-grand-central-companions-multi-instantiation
branch
from
7c1a1d7 to
19a9de6
Compare
seldridge
deleted the
dev/seldridge/firrtl-allow-grand-central-companions-multi-instantiation
branch
Change the Grand Central View lowering pass to now allow for Grand Central
Companions to be multiply instantiated. This should have technically been
possible for a long time [1] as long as there was a guarantee that there
was never going to be a leaf that was outside the companion.
This is a pretty simple change to just loop over all instances and extract
(add a
lowerToBindattribute) for the ones which are in the design. Theother, subtle blocker is that without the
InstanceInfo, doing the checkof "in the design" was tedious. Now this is pretty straightforward.