Co-authored-by: Andrew Lenharth <andrew@lenharth.org>
Co-authored-by: Fabian Schuiki <fabian@schuiki.ch>
Co-authored-by: Andrew Young <youngar17@gmail.com>
This improves verilog emission quality because it allows expressions
to be inlined into input ports of the module. This eliminates some
extranous wires, shrinking `wc -l` on a big testcase by 3%. This
fixes Issue #1568
This generalizes the previous patch to sink expressions with multiple
users to the deepest common region between all the users. In the case
of FIRRTL, this sinks a shocking amount of test-only code into
"ifndef SYNTHESIS" blocks, eliminating wires at the top level.
However, because these expressions have multiple uses, they don't get
emitted inline - they get temporaries declared at their local scope,
usually as `automatic logic` values. These never get their initializer
emitted inline (see Issue #1567), but sinking these is still the right
thing to do IMO.
This allows the verilog printer to print them inline, for example we'd
turn the example from Issue #1601 into:
```
module test(
input clock, a, a2, a3);
wire _T = a2 | a3;
always @(posedge clock) begin
`ifndef SYNTHESIS
if (`PRINTF_COND_ & a)
$fwrite(32'h80000002, "thing");
if (`STOP_COND_ & _T)
$fatal;
`endif
end // always @(posedge)
endmodule
```
We now produce: `if (`STOP_COND_ & (a2 | a3))` with no temporary
wire. This is a pretty huge improvement to lots of things.
There is a more general version of this as well, but it seems best
to split into two patches.
Different build configurations were previously trampling on the same
cache directory on the build host. The cache directory wasn't
configurable, so just use the cache action directly.
This allows it to compose with the binary expression tree lowering
work that is already there, allowing us to handle multi-operand
additions that end with a constant.
This pass is duplicating unary ops in order to make verilog prettier,
but can result in the op being in a different block than the constant.
This causes the constant to get emitted as a weird local param, which
can even be unused in the generated verilog. Fix this to duplicate
the constant operands as well.
* [Calyx] Add IR support for calyx::IfOp
This commit adds support for the Calyx If control operation. The operation takes two arguments; a b boolean SSA value and a group name. The op has two regions, a mandatory 'then' and optional 'else' region.
This is solely for IR support and does not include:
- CalyxEmitter support (unsupported for all control ops as of now)
- CompileControl control FSM generation
When emitting two always/initial/... blocks with the same conditions, leave the
resultant block at the location of the last use, instead of at the location of
the first use. This ensures that the values used by that block are defined above
it.
This isn't necessary by the semantics of our IR, since hw.module is a graph region,
but leads to better generated verilog and puts less pressure on the cleanup passes.
Add a `--parse-only` option to `firtool` which causes the program to
stop after the FIR/MLIR and annotation input files have been parsed and
processed, and writes the resulting MLIR module to the output. This is
interesting and useful since `firtool` performs a unique combination of
input translation and annotation scattering that is not trivially
reproduced with `circt-translate` and `circt-opt`. Useful for test case
reduction.
- Update/rewrite the `circt-reduce` tool with a custom proof-of-concept
reducer for the FIRRTL dialect. This is supposed to be a pathfinding
exercise and just uses FIRRTL as an example. The intent is for other
dialects to be able to produce sets of their own reducers that will
then be combined by the tool to operate on some input IR.
At this point, `circt-reduce` can be used to reduce FIRRTL test cases by
converting as many `firrtl.module` ops into `firrtl.extmodule` ops as
possible while still maintaining some interesting characteristic.
- Extend `circt-reduce` to support exploratively applying passes to the
input in an effort to reduce its size. Also add the ability to specify
an entire list of potential reduction strategies/patterns which are
tried in order. This allows for reductions with big effect, like
removing entire modules, to be tried first, before taking a closer
look at individual instructions.
- Add reduction strategies to `circt-reduce` that try to replace the
right hand side of FIRRTL connects with `invalidvalue`, and generally
try to remove operations if they have no results or no users of their
results.
- Add a reduction pattern to `circt-reduce` that replaces instances of
`firrtl.extmodule` with a `firrtl.wire` for each port. This can
significantly reduce the complexity of test cases by pruning the
module hierarchy of unused modules.
- Move the `Reduction` class and sample implementations into a separate
header and implementation file. These currently live in
`tools/circt-reduce`, but should later move into a dedicated reduction
framework, maybe in `include/circt/Reduce`, where dialects can easily
access them and provide their own reduction implementations.
When a node is used as the address (aka index) of a CHIRRTL synchronous
read-only memory port, the memory port is enabled at the declaration
location of the node op. Nodes are being removed by the parser if they
don't carry any annotations. When the node op is removed by the parser,
the enable conditions of the memory change, and sometimes the memory
port is never enabled.
This change removes the small optimization from the FIRParser so that
the memory port enable can be properly inferred. These node operations
will still be removed later on during canonicalization.
The two CHIRRTL memory operations have been renamed from `smem` to
`seqmem` and `cmem` to `combmem`. In addition, instead of returning
`!firrtl.vector<>` types, they return a new type `!firrtl.cmemory`.
This new type can only be used with CHIRRTL memories and ports, and
prevents some shenanigans where it could be used like a normal vector.
Memory ports in SFC are allowed to be used outside of the scope which they are
defined in. To work around this issue, the memory port declaration was split
into two operations:
1. `firrtl.memoryport`: This operation is the declaration of the memory port,
and it should be emitted into the body of the module.
2. `firrtl.memoryport.acccess`: This operations is emitted to the location of
the original memory port declaration, and is used for enable inference.
For more information about these new operations, and why we added them, see the
changes to FIRRTLRational.md.
This include changes to the FIR parser to emit these new operations.
Schuyler points out in Issue #1600 that it isn't correctly implemented,
and there is only one place in the compiler that forms it ... in a
theoretical case. I added this a long time ago on a theoretical basis.
It is best to remove this until there is a real need for it.
This fixes Issue #1600.
This enables LowerToHW's auto-cse of always blocks to kick in in more
cases, e.g. in the example from Issue #1594. Thanks to Schuyler for
tracking down the root issue here.
The Cell trait is used to annotate each sub-component within a component.
This consists of primitives (e.g. RegisterOp) and component instances. This will
be useful for future passes that want to work on Cells, such as resource-sharing.
I've also renamed the CellOp to InstanceOp.
Modify sv-dialect.mlir to generate legal Verilog, with properly defined
`defines so that the output can be directly checked with a linter.
Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
Fix a bug where a spilled localparam could result in invalid Verilog (a
two-statement always block without a begin/end).
Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
Change sv-dialect test to emit legal Verilog (a comment) from an
sv.verbatim op as opposed to raw strings that are illegal Verilog.
Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
Fix an issue where we would iterate over `op->getUses()` for each
`InvalidValueOp` to create a unique instance of the op for every use.
That iterator gets invalidated by the modifications we make though,
which caused only the second (yes, exactly the second) use to be
properly uniquified, with all others remaining the same.
`llvm::make_early_inc_range` to the rescue.
Thanks to @youngar for reducing this down from a much larger test case
and doing the awesome work of hunting this down in a >70M source file.
Add the reset network name to error messages about conflicting async and
sync drives to the same network. They now read something like:
error: reset network "foo.io.reset" simulatenously connected to async
and sync resets
Fix issues related to zero width behavior involving trailing commas and
end of instance declarations.
Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
Block inlining of ArrayCreate into ArrayGet users. This prevents
ExportVerilog from producing invalid Verilog. Add one test that locks
in this behavior and update existing tests that incorrectly expected
this.
Fixes#1587.
Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
Expand whens takes the following code:
```firrtl
when a:
x <= 1
else
x <= 2
```
And transforms it turns it into the (desirable) code:
```firrtl
x <= mux(a, 1, 2)
```
However for nested whens, it was being overly pessimistic in the
condition of the mux.
Expand whens takes the following code:
```firrtl
when a:
when b:
x <= 1
else:
x <= 2
else
x <= 3
```
And transforms it into
```firrtl
x <= mux(a, mux(a & b, 1, 2), 3)
```
When it should transform it into:
```firrtl
x <= mux(a, mux(b, 1, 2), 3)
```
In the code change `condition` refers to the condition of the current
WhenOp , and `thenCondition` refers to the all parent `WhenOp`
conditions `and`ed together. The `thenCondition` is intended to be used
for simulation constructs which are copied directly into the module's
body.
The following example shows how `thenCondition` is used:
```firrtl
when a:
when b:
printf(clock, UInt<1>("h1"), "hi")
```
Here, `thenCondition` is `a & b`:
```firrtl
printf(clock, a & b & 1, "hi")
```
When tracing the resets of a connect operation, the pass would
recursively trace the src and dest values. When one of the values came
from a subfield operation, it would associate the resets of the input
bundle of the subfield operation with the resets in result. The pass
neglected to recursively trace the resets of the input of each subfield
operation, which resulted in a failure to determine the correct
reset-network when there were nested aggregates.
To fix this, we need to recursively trace the resets of the input
expression of every subfield operation.
This strategy of recursively visiting the src and dest expressions of a
connect statement probably comes from the SFC, where the result of a
subfield operation can only be used once. In MFC this could result in
visiting the same subfield operation multiple times. It is more
efficient to handle subfield and subindex operations as we walk through
the module. By handling each subfield operation as we see it, we don't
need to recursively trace subfield operations.
Also update the types of extmodule ports that were inferred during reset
inference. In general we do not want to update the type of extmodules,
since these are provided from the outside and the interface is thus
fixed. However in the case of reset inference where the types all map to
the same Verilog, this is okay to do.
This currently uses the types inferred for `InstanceOp`s to determine
what types the corresponding extmodule should have. This works in the
cases we are interested in, but can break in some corner cases where a
reset network is only connected through an extmodule port. In that case,
since we don't have a `Value` for that extmodule port, the two reset
networks can remain disjoint.
This fixes#1578.
The presence of `InvalidValueOp` is problematic during reset inference.
Generally, CSE will ensure that there is at most one `InvalidValueOp` of
a given type. However, if that op is of `ResetType`, it might be
connected to multiple reset networks as a `rst is invalid` connection.
Currently, `InferResets` will consider these reset networks to become
connected along this `InvalidValueOp`, which is incorrect.
This commit adds a `InvalidValueOp` uniquification step to the beginning
of reset inference, which replicates these ops such that they have at
most one use. This is necessary, since that single `ResetType` invalid
value may become part of a `AsyncResetType` and a `UIntType` reset
network, in which case two distinct invalid values are required.
This fixes an issue uncovered by @youngar and @Ramlakshmi3733.
This takes the example in Issue #1560 from:
```
assign a = (auto_in_aw_bits_addr[37:29] ^ 9'h2) == 9'h0;
```
to:
```
assign a = auto_in_aw_bits_addr[37:29] == 9'h2;
```
which is a heck of a lot better than the starting point of:
```
assign a = ~(|({1'h0, auto_in_aw_bits_addr ^ 38'h40000000} & 39'h7FE0000000));
```
This now turns things like `(and x, 3) == 1` into `extract x[1:0] == 1`.
This composes very nicely with extract shrinking, but extract shrinking
needs to be taught some new tricks.
John Demme