Skip to content

[ExportVerilog] Smarter handling of large expression splitting and spilling. #2810

Open
@mikeurbach

Description

@mikeurbach

We have logic related to splitting large expressions into multiple lines, as well as spilling expressions into wires. This is split between both ExportVerilog and PrepareForEmission. We should clean this up, and handle splitting and spilling into wires in PrepareForEmission.

Currently, PrepareForEmission does two things here: first, it looks at variadic expressions, and spills them to their own wire in the top-level module body, when that is possible:

// If this expression is deemed worth spilling into a wire, do it here.
if (shouldSpillWire(op, options)) {
// If we're not in a procedural region, or we are, but we can hoist out of
// it, we are good to generate a wire.
if (!isProceduralRegion ||
(isProceduralRegion && hoistNonSideEffectExpr(&op))) {
lowerUsersToTemporaryWire(op);
// If we're in a procedural region, we move on to the next op in the
// block. The expression splitting and canonicalization below will
// happen after we recurse back up. If we're not in a procedural region,
// the expression can continue being worked on.
if (isProceduralRegion) {
++opIterator;
continue;
}
}
}

Then, it splits up the variadic expressions into a binary tree of expressions, which makes it easier to break them up into multiple lines in ExportVerilog:

// Lower variadic fully-associative operations with more than two operands
// into balanced operand trees so we can split long lines across multiple
// statements.
// TODO: This is checking the Commutative property, which doesn't seem
// right in general. MLIR doesn't have a "fully associative" property.
if (op.getNumOperands() > 2 && op.getNumResults() == 1 &&
op.hasTrait<mlir::OpTrait::IsCommutative>() &&
mlir::MemoryEffectOpInterface::hasNoEffect(&op) &&
op.getNumRegions() == 0 && op.getNumSuccessors() == 0 &&
(op.getAttrs().empty() ||
(op.getAttrs().size() == 1 && op.hasAttr("sv.namehint")))) {
// Lower this operation to a balanced binary tree of the same operation.
SmallVector<Operation *> newOps;
auto result = lowerFullyAssociativeOp(op, op.getOperands(), newOps);
op.getResult(0).replaceAllUsesWith(result);
op.erase();
// Make sure we revisit the newly inserted operations.
opIterator = Block::iterator(newOps.front());
continue;
}

We should improve and combine this logic. The checking for what is "too large" should be considering the entire expression tree, and inserting splits and wires according to the defined expression term limit. It should also keep the (sub-)expressions neat, so that ExportVerilog can easily break them into multiple lines, which may require further splitting into a binary tree as is done now.

After we've finished that, we can remove the old large expression spilling logic from ExportVerilog once and for all: #2802

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions