Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
mikeurbach opened this issue Mar 25, 2022 · 0 comments
Assignees
Labels
enhancement New feature or request ExportVerilog

Comments

@mikeurbach
Copy link
Contributor

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

@mikeurbach mikeurbach added enhancement New feature or request ExportVerilog labels Mar 25, 2022
@mikeurbach mikeurbach self-assigned this Mar 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request ExportVerilog
Projects
None yet
Development

No branches or pull requests

1 participant