Change the `AddSeqMemPorts` pass (which adds MBIST ports to memories) to
use a walk when adding ports to memories. This will visit any region, but
will skip memories which instantiated under LayerBlockOps. Memories and
instances which are instantiated under WhenOps will now get ports added to
them. Any output ports created for a memory under a WhenOp will be
invalidated.
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 <schuyler.eldridge@sifive.com>
LowerLayers belongs with the other passes that lower out of FIRRTL.
Start moving it, now that first batch of passes have been fixed
to handle enough FIRRTL IR to run in presence of layer operations.
inlineInstances/flattenInstances:
* Walk entire body, not only top-level operations.
Fixes missing instances and allows inlining them
when conservatively legal.
* Reject inlining instances under when/match.
inlineInto/flattenInto:
Walk entire body using new `inliningWalk` method
that drives the per-operations handling but also
handles cloning "structure" operations that have
regions (when/match/layer) and managing what
should be cloned where.
This allows inlining modules that contain these
operations.
Inliner now may produce errors, thread throughout.
This allows the inliner to run earlier in the pipeline,
particularly before LowerLayers.
Add a CircuitOp verifier that guarantees that there is zero or one modules
annotated with a `MarkDUTAnnotation`. This is important as many passes
assume that this is the case. Moving this check into a verifier means
that logic checking this behavior in each pass can be removed. I am
slightly concerned of the performance cost of doing this as examining the
annotations is non-trivial and this will cause that examination to now
happen more often.
Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
Simply skip the elaboration system tasks `$info`, `$warning`, `$error`,
and `$fatal` outside procedural code. Slang already processes these
tasks and generates the corresponding diagnostics for us.
Slap a `maybe_unused` on an unused variable to silence a compiler warning.
There may be a cleaner way to do this with a `resize`/`std::fill` pattern
rather than going the unused route.
Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
Remove the `NamedConstantOp` and replace its uses with `VariableOp` from
the debug dialect.
The op was originally added to track the value of constant parameters,
localparams, and specparams in the IR. In ImportVerilog, such parameters
would generate a corresponding `named_constant` op and all references to
the parameter by name would be replaced with the `named_constant`'s
result.
This doesn't really work well for parameters defined outside a module,
such as in packages or at the root of the Verilog source file. (Modules
are isolated from above, preventing the use of `named_constant`s from
outside the module.) Therefore expressions would generally fall back to
materializing constants directly where they were used.
Since the named constant ops are only there to track a constant value in
the IR for the user's debugging convenience, using the debug dialect
directly feels a lot more appropriate.
There has been MacroRefExprOp and MacroRefExprSEOp to use macro symbols as expressions but it was not possible to use macro as a statement. This commit adds `sv.macro.ref` op to represent a statement. Since `sv.macro.ref` mnemonic was used by MacroRefExprOp this commit also renames `sv.macro.ref` to `sv.macro.ref.expr` at the same time.
Change FIRRTL's `InstanceInfo` analysis to use `llvm::inverse_post_order`
instead of `ReversePostOrderTraversal`. This may be slightly faster/more
efficient as it doesn't build a vector of what to visit in a post order
walk and this will visit all modules, not just those under the top module.
h/t @youngar for the pattern and the reasoning behind why to favor this
pattern.
Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
Change the `LowerMemory` pass to use the `InstanceInfo` analysis to
determine when doing computations about what is under vs. not under the
design-under test.
Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
Add an option to ImportVerilog to enable debug info generation. If set,
the Verilog importer should generate additional `dbg.*` ops in the IR to
track variables and constants defined by the user in the Verilog source
text. No such ops are generated yet.
Also add a corresponding `-g` option to `circt-verilog`.
Factor the materialization of Slang `SVInt`s and `ConstantValue`s out
into a helper function on the import context. This will allow for easier
access to constant materialization from outside the expression
conversion code.
Add support for the Debug dialect to the MooreToCore conversion. The
conversion simply updates the types of `dbg.variable`, `dbg.array`, and
`dbg.struct` as the values they observe are being lowered. Debug ops
referring to Moore-typed values will refer to the corresponding HW-typed
values after conversion.
Change the functions for the `InstanceInfo` analysis to use "any" instead
of "atLeastOne". The former is far simpler and better aligns with the
"all" member functions.
Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
Add two new functions for computing if a module is under the "effective"
design-under-test (DUT):
- atLeastOneInstanceUnderEffectiveDut
- allInstancesUnderEffectiveDut
These functions are very simple. However, their use comes up in a number
of passes and this makes _reading_ those passes more straightforward.
I.e., it's quicker to grok what is going on with these functions than with
`!hasDut() || atLeastOneInstanceUnderEffectiveDut()`.
Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
Change the location where the `LowerMemory` pass creates modules from
creating modules at the end of the circuit to inserting them just above
the module where they have a first user. This has two benefits: (1) this
is a more natural place to insert these modules (a minor IR/Verilog
quality concern) and (2) this avoids revisiting these modules
unnecessarily because the pass is doing a linear walk of the circuit from
top to bottom.
The latter benefit avoids some potential problems where an analysis that
is computed before the modules gets inserted has no knowledge of the new
modules. Any queries of these analyses may then crash and burn. Namely,
this affects switching this pass to use the `InstanceInfo` analysis and
avoiding precomputation of which modules are under the
design-under-test (DUT).
This should have no effect on names in the final Verilog since module
names (symbol names) must be unique. I.e., the order of FIRRTL modules
won't affect final module names.
Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
[FIRRTL] Add features, cleanup InstanceInfo (again)
This fixes problems with a commit [[1]] that was reverted [[2]]. The commit
message below is unchanged.
Add a number of new features to FIRRTL's `InstanceInfo` analysis. These
changes were made after downstream efforts (uncommitted work) to use this
in the `AddSeqMemPorts` pass. These new features include:
- `hasDut` to check if a circuit has a design-under-test
- `getDut` to get the design-under-test if it exists
- `getEffectiveDut` to get the "effective" design-under-test
- `isEffectiveDut` to check if a module is the "effective"
design-under-test
The "effective" design-under-test (DUT) is a weird artifact of how some
SFC-derived passes have historically worked. If a circuit has no
DUT (indicated by a circuit which contains no module annotated with a
`MarkDUTAnnotation`), then some passes will treat the top module as if it
were the DUT. This "effective" DUT concept shows up in `AddSeqMemPorts`,
`GrandCentral`, and other passes. For now, enshrine the _computation_ of
this in the `InstanceInfo` analysis to avoid having to scatter logic for
computing this across serverl passes.
Widen the accepted type of parameters that can be passed to `InstanceInfo`
member functions from `FModuleOp` to `igraph::ModuleOpInterface`. This
already works without modifications and makes these member functions much
more usable.
[1]: 7e9cedd07
[2]: 2683f6772
Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
This reverts commit 7e9cedd07b. This commit
is showing problems with short integration tests hanging. I'm reverting
it temporarily to prevent any issues from leaking into main.
Add a number of new features to FIRRTL's `InstanceInfo` analysis. These
changes were made after downstream efforts (uncommitted work) to use this
in the `AddSeqMemPorts` pass. These new features include:
- `hasDut` to check if a circuit has a design-under-test
- `getDut` to get the design-under-test if it exists
- `getEffectiveDut` to get the "effective" design-under-test
- `isEffectiveDut` to check if a module is the "effective"
design-under-test
The "effective" design-under-test (DUT) is a weird artifact of how some
SFC-derived passes have historically worked. If a circuit has no
DUT (indicated by a circuit which contains no module annotated with a
`MarkDUTAnnotation`), then some passes will treat the top module as if it
were the DUT. This "effective" DUT concept shows up in `AddSeqMemPorts`,
`GrandCentral`, and other passes. For now, enshrine the _computation_ of
this in the `InstanceInfo` analysis to avoid having to scatter logic for
computing this across serverl passes.
Widen the accepted type of parameters that can be passed to `InstanceInfo`
member functions from `FModuleOp` to `igraph::ModuleOpInterface`. This
already works without modifications and makes these member functions much
more usable.
Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
Add a new InstanceInfo analysis. This analysis provides common
information which can be computed from a single walk of the InstanceGraph.
The idea behind this is that there are a large amount of common
information that different passes want to compute which all follows this
same walk (and therefore has the same algorithmic complexity to compute
it). Move all this information into a single, common analysis.
This analysis currently enables O(1) queries of if something is the DUT,
is instantiated under the DUT, or is instantiated under a layer. More
queries can be added as they are identified.
This new analysis naturally depends on the InstanceGraph analysis.
Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
The existing path support was built up based on the assumption that
every target is unique. That is true for FIRRTL produced by standard
Chisel code, which elaborates unique modules for each instance. We
definitely don't want to limit ourselves to this world, and we should
support targeting things that are multiply instantiated when it is not
ambiguous what we refer to.
This patch relaxes the single-instantiation constraints for local
targets, which refer to a module or something inside a module,
regardless of how many times or at what paths that particular module
was instantiated.
This required a couple changes through the pipeline:
ResolvePaths already had an early exit for the local path case, but
this needed to come before the single-instantiation check.
LowerClasses needed a couple small changes to not enforce the
single-instantiation check in the local path case, and to build a
hierpath that just has a single element.
While this is not a new requirement, we can still get ambiguous local
targets, for instance from nested module prefixing. The error message
in LowerClasses for this case was made a little more clear.
Move some code which was in the MSFT dialect (which used to be used both there
and in ESI) into the ESI pass which actually uses it. Also, reorg the code in
the services impl.
Remove an unused utility function. This was previously used by
GrandCentral, but this is no longer load bearing.
Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
IMCP needs to use a post-order walk as it is deleting operations (and the
operations are not skipped after erasure). This should fix observed
failures in nightly CI [[1]].
[1]: 46014cd277 (commitcomment-146717821)
Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
Change IMCP to recurse into layers. This has the effect of allowing IMCP
to sink constants into layers and to properly visit instances which are
instantiated under layers.
Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
Change IMCP's module update to use a walk instead of only visiting
top-level ops. This is intended to be an entirely mechanical change and
is broken out in a separate commit because of its mechanical nature.
Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
Fix incorrect iteration over a set to do operation erasure. The order of
operations is operations is important to avoid erasing an op before all
its users are erased. Avoid this with a SetVector.
h/t @dtzSiFive for identifying the problem/fix.
Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
Change the InferReadWrite pass to fail if it ever sees a WhenOp. If this
happens, the pass can silently do the wrong thing when trying to determine
signal drivers. This is a conservative check (with false positives), but
will have no false negatives.
Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
Fix the InferReadWrite pass to walk into nested regions instead of only
visiting the top-level ops immediately in an FModuleOp's body. This has
the effect of allowing it to work on memories that are declared inside
layers.
Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
Change LowerMemory to use a walk as opposed to iteration over operations.
This is done to make this pass work with operations which have regions and
may contain memories, i.e., to make this pass work with layers and whens.
Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
Schuyler Eldridge