[FIRRTL] Use walk, InstanceInfo in AddSeqMemPorts #7599
Conversation
|
This PR was changed to use a walk, but to skip over memories that are wholly instantiated under layer blocks. Memories that are partially instantiated under layer blocks will error. |
seldridge
force-pushed
the
dev/seldridge/layers-add-seq-mem-ports
branch
2 times, most recently
from
acd26fe
to
6c362e0
Compare
seldridge
changed the title
seldridge
force-pushed
the
dev/seldridge/layers-add-seq-mem-ports
branch
from
6c362e0
to
a998fbc
Compare
Change the `AddSeqMemPorts` pass (whic adds MBIST ports to memories) to use a walk when adding ports to memories. Practically, this causes the pass to now be capable of adding MBIST to memories under `WhenOp`s or `LayerBlockOp`s. However, because this pass is intentionally _not_ supposed to add MBIST ports to anything that is not in the "design", this will skip over any layers or modules that are instantiated wholly under layers. To do this, this pass was changed to use the new `InstanceInfo` analysis instead of computing information about what modules are under the design-under-test (DUT) manually. Any checking related to layers is similarly done using this analysis. This change is prerequisite work to move the `LowerLayers` after `AddSeqMemPorts` while still preserving the functionality of the original pass order. Unfortunately, layers do not compose sanely with `AddSeqMemPorts`. This is a failing of the original pass and its assumptions about what is "design" vs. "verification" as opposed to any fundamental issue with layers. Generally, layers are intended as verification features. However, there is no actual requirement or expectation that they will be used this way. Going forward, `AddSeqMemPorts` is intended to be removed and replaced with a feature in Chisel. Additionally, this this commit includes a small change to the pass output where the connects created are placed immediately following the memory instance. This is necessary to create the connects inside the layer blocks. However, it is also slightly better output than before. (This changed one test superficially.) Signed-off-by: Schuyler Eldridge <[email protected]>
seldridge
force-pushed
the
dev/seldridge/layers-add-seq-mem-ports
branch
from
914e069
to
ed23a81
Compare
|
|
||
| // Add the extra ports to this module. | ||
| moduleOp.insertPorts(extraPorts); | ||
|
|
||
| // Connect the submodule ports to the parent module ports. | ||
| DenseMap<Operation *, OpBuilder::InsertPoint> instToInsertionPoint; |
There was a problem hiding this comment.
Maybe you could walk the values in reverse, or change the storage of the values to group by instance? or just ignore me and leave your patch as-is :)
There was a problem hiding this comment.
I like this idea. I realized that this change was now very out-of-place for this PR. This PR doesn't need this anymore (as it is not creating connects under layer blocks). I opted instead to just back this out of the patch in b11b072.
There was a problem hiding this comment.
I'm going to leave this for a follow-up as this would permute the code a bit too much in this PR. I like the idea.
| if (instanceInfo->allInstancesUnderLayer(op)) | ||
| continue; |
There was a problem hiding this comment.
If the DUT is wholly under a layer, then we won't add any MBIST ports. Seems OK.
There was a problem hiding this comment.
That seems reasonable, though I suppose it could work? E.g., there's no problem wiring to the DUT as long as you don't have to pass through any intermediary layers to do it. (InstanceInfo doesn't track that, though!)
This smells like a missing check that should be in LowerAnnotations (as I don't think we can do it with a verifier).
This reverts commit b11b072.
Make AddSeqMemPorts work for memories under WhenOps. This takes a necessary interpretation of how this should work which may create disagreement. However, it is the only reasonable thing to do. Namely, for a memory that is under a WhenOp that is having _output_ ports added to it, this will drive the port under the WhenOp and invalidate it at the top of the module. This interpretation is that the MBIST outputs are only driven under the WhenOp and indeterminate otherwise. CIRCT will treat this as an unconditional connect after ExpandWhens, however, the spec allows for it to connect it to anything.
Change the
AddSeqMemPortspass (whic adds MBIST ports to memories) touse a walk when adding ports to memories. Practically, this causes the
pass to now be capable of adding MBIST to memories under
WhenOps orLayerBlockOps. However, because this pass is intentionally notsupposed to add MBIST ports to anything that is not in the "design", this
will skip over any layers or modules that are instantiated wholly under
layers.
To do this, this pass was changed to use the new
InstanceInfoanalysisinstead of computing information about what modules are under the
design-under-test (DUT) manually. Any checking related to layers is
similarly done using this analysis.
This change is prerequisite work to move the
LowerLayersafterAddSeqMemPortswhile still preserving the functionality of the originalpass order. Unfortunately, layers do not compose sanely with
AddSeqMemPorts. This is a failing of the original pass and itsassumptions about what is "design" vs. "verification" as opposed to any
fundamental issue with layers. Generally, layers are intended as
verification features. However, there is no actual requirement or
expectation that they will be used this way. Going forward,
AddSeqMemPortsis intended to be removed and replaced with a feature inChisel.
Additionally, this this commit includes a small change to the pass output
where the connects created are placed immediately following the memory
instance. This is necessary to create the connects inside the layer
blocks. However, it is also slightly better output than before. (This
changed one test superficially.)