Skip to content

Commit

Permalink
[ExportVerilog] Implement 'disallowLocalVariables' for non-SV compati…
Browse files Browse the repository at this point in the history 
…bility.

This implements `disallowLocalVariables` lowering option with a simple approach:
have ExportVerilog move roughly all expressions to the top level.  This ensures
we don't need to generate automatic temporary variables, which aren't compatible
with classic-verilog consumers.
  • Loading branch information
@lattner
lattner committed Aug 31, 2021
1 parent 34832f4 commit 05c7a13
Show file tree
Hide file tree
Showing 3 changed files with 158 additions and 36 deletions.
15 changes: 10 additions & 5 deletions lib/Translation/ExportVerilog/ExportVerilog.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -280,7 +280,8 @@ static void printUnpackedTypePostfix(Type type, raw_ostream &os) {
}

/// Return the word (e.g. "reg") in Verilog to declare the specified thing.
static StringRef getVerilogDeclWord(Operation *op) {
static StringRef getVerilogDeclWord(Operation *op,
const LoweringOptions &options) {
if (isa<RegOp>(op)) {
// Check if the type stored in this register is a struct or array of
// structs. In this case, according to spec section 6.8, the "reg" prefix
Expand Down Expand Up @@ -313,6 +314,10 @@ static StringRef getVerilogDeclWord(Operation *op) {
// If 'op' is in a module, output 'wire'. If 'op' is in a procedural block,
// fall through to default.
bool isProcedural = op->getParentOp()->hasTrait<ProceduralRegion>();

// "automatic logic" values aren't allowed in disallowLocalVariables mode.
assert((!isProcedural || !options.disallowLocalVariables) &&
"automatic variables not allowed");
return isProcedural ? "automatic logic" : "wire";
}

Expand Down Expand Up @@ -1589,8 +1594,8 @@ void NameCollector::collectNames(Block &block) {
valuesToEmit.push_back(ValuesToEmitRecord{result, {}});
auto &typeString = valuesToEmit.back().typeString;

maxDeclNameWidth =
std::max(getVerilogDeclWord(&op).size(), maxDeclNameWidth);
StringRef declName = getVerilogDeclWord(&op, moduleEmitter.state.options);
maxDeclNameWidth = std::max(declName.size(), maxDeclNameWidth);

// Convert the port's type to a string and measure it.
{
Expand Down Expand Up @@ -2700,7 +2705,7 @@ isExpressionEmittedInlineIntoProceduralDeclaration(Operation *op,
/// return false. If the operation *is* a constant, also emit the initializer
/// and semicolon, e.g. `localparam K = 1'h0`, and return true.
bool StmtEmitter::emitDeclarationForTemporary(Operation *op) {
StringRef declWord = getVerilogDeclWord(op);
StringRef declWord = getVerilogDeclWord(op, state.options);

// If we're emitting a declaration inside of an ifdef region, we'll insert
// the declaration outside of it. This means we need to unindent a bit due
Expand Down Expand Up @@ -2766,7 +2771,7 @@ void StmtEmitter::collectNamesEmitDecls(Block &block) {

// Emit the leading word, like 'wire' or 'reg'.
auto type = record.value.getType();
auto word = getVerilogDeclWord(op);
auto word = getVerilogDeclWord(op, state.options);
if (!isZeroBitType(type)) {
indent() << word;
auto extraIndent = word.empty() ? 0 : 1;
Expand Down
124 changes: 95 additions & 29 deletions lib/Translation/ExportVerilog/PrepareForEmission.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -236,9 +236,10 @@ static void lowerUsersToTemporaryWire(Operation &op) {
}
}

/// Transform "a + -cst" ==> "a - cst" for prettier output.
static void rewriteAddWithNegativeConstant(comb::AddOp add,
hw::ConstantOp rhsCst) {
/// Transform "a + -cst" ==> "a - cst" for prettier output. This returns the
/// first operation emitted.
static Operation *rewriteAddWithNegativeConstant(comb::AddOp add,
hw::ConstantOp rhsCst) {
ImplicitLocOpBuilder builder(add.getLoc(), add);

// Get the positive constant.
Expand All @@ -248,6 +249,18 @@ static void rewriteAddWithNegativeConstant(comb::AddOp add,
add.erase();
if (rhsCst.use_empty())
rhsCst.erase();
return negCst;
}

/// Given an operation in a procedural region, scan up the region tree to find
/// the first operation in a graph region (typically an always or initial op).
static Operation *findParentInNonProceduralRegion(Operation *op) {
Operation *parentOp = op->getParentOp();
assert(parentOp->hasTrait<ProceduralRegion>() &&
"we should only be hoisting from procedural");
while (parentOp->getParentOp()->hasTrait<ProceduralRegion>())
parentOp = parentOp->getParentOp();
return parentOp;
}

/// This function is invoked on side effecting Verilog expressions when we're in
Expand All @@ -260,11 +273,6 @@ static void rewriteAddWithNegativeConstant(comb::AddOp add,
static bool rewriteSideEffectingExpr(Operation *op) {
assert(op->getNumResults() == 1 && "isn't a verilog expression");

// If the operation is in a non-procedural region (e.g. top level or in an
// `ifdef at the top level), then leave it alone.
if (!op->getParentOp()->hasTrait<ProceduralRegion>())
return false;

// Check to see if this is already rewritten.
if (op->hasOneUse()) {
if (auto assign = dyn_cast<BPAssignOp>(*op->user_begin()))
Expand All @@ -278,10 +286,7 @@ static bool rewriteSideEffectingExpr(Operation *op) {
Value opValue = op->getResult(0);

// Scan to the top of the region tree to find out where to insert the reg.
Operation *parentOp = op->getParentOp();
while (parentOp->getParentOp()->hasTrait<ProceduralRegion>())
parentOp = parentOp->getParentOp();

Operation *parentOp = findParentInNonProceduralRegion(op);
OpBuilder builder(parentOp);
auto reg = builder.create<RegOp>(op->getLoc(), opValue.getType());
builder.setInsertionPointAfter(op);
Expand All @@ -295,11 +300,57 @@ static bool rewriteSideEffectingExpr(Operation *op) {
return true;
}

/// This function is called for non-side-effecting Verilog expressions when
/// we're in 'disallowLocalVariables' mode for old Verilog clients. It hoists
/// non-constant expressions out to the top level so they don't turn into local
/// variable declarations.
static bool hoistNonSideEffectExpr(Operation *op) {
// Scan to the top of the region tree to find out where to move the op.
Operation *parentOp = findParentInNonProceduralRegion(op);

// We can typically hoist all the way out to the top level in one step, but
// there may be intermediate operands that aren't hoistable. If so, just
// hoist one level.
bool cantHoist = false;
if (llvm::any_of(op->getOperands(), [&](Value operand) -> bool {
// The operand value dominates the original operation, but may be
// defined in one of the procedural regions between the operation and
// the top level of the module. We can tell this quite efficiently by
// looking for ops in a procedural region - because procedural regions
// live in graph regions but not visa-versa.
Operation *operandOp = operand.getDefiningOp();
if (!operandOp) // References to ports are always ok.
return false;

if (operandOp->getParentOp()->hasTrait<ProceduralRegion>()) {
cantHoist |= operandOp->getBlock() == op->getBlock();
return true;
}
return false;
})) {

// If the operand is in the same block as the expression then we can't hoist
// this out at all.
if (cantHoist)
return false;

// Otherwise, we can hoist it, but not all the way out in one step. Just
// hoist one level out.
parentOp = op->getParentOp();
}

op->moveBefore(parentOp);
return true;
}

/// For each module we emit, do a prepass over the structure, pre-lowering and
/// otherwise rewriting operations we don't want to emit.
void ExportVerilog::prepareHWModule(Block &block, ModuleNameManager &names,
const LoweringOptions &options,
const SymbolCache &cache) {
// True if these operations are in a procedural region.
bool isProceduralRegion = block.getParentOp()->hasTrait<ProceduralRegion>();

for (Block::iterator opIterator = block.begin(), e = block.end();
opIterator != e;) {
auto &op = *opIterator++;
Expand All @@ -310,13 +361,6 @@ void ExportVerilog::prepareHWModule(Block &block, ModuleNameManager &names,
prepareHWModule(region.front(), names, options, cache);
}

// Duplicate "always inline" expression for each of their users and move
// them to be next to their users.
if (isExpressionAlwaysInline(&op)) {
lowerAlwaysInlineOperation(&op);
continue;
}

// Lower variadic fully-associative operations with more than two operands
// into balanced operand trees so we can split long lines across multiple
// statements.
Expand All @@ -343,7 +387,8 @@ void ExportVerilog::prepareHWModule(Block &block, ModuleNameManager &names,
if (auto cst = addOp.getOperand(1).getDefiningOp<hw::ConstantOp>()) {
assert(addOp.getNumOperands() == 2 && "commutative lowering is done");
if (cst.getValue().isNegative()) {
rewriteAddWithNegativeConstant(addOp, cst);
Operation *firstOp = rewriteAddWithNegativeConstant(addOp, cst);
opIterator = Block::iterator(firstOp);
continue;
}
}
Expand Down Expand Up @@ -401,22 +446,43 @@ void ExportVerilog::prepareHWModule(Block &block, ModuleNameManager &names,
}
}

// Force any side-effecting expressions in nested regions into a sv.reg
// if we aren't allowing local variable declarations. The Verilog emitter
// doesn't want to have to have to know how to synthesize a reg in the case
// they have to be spilled for whatever reason.
if (options.disallowLocalVariables && op.getNumResults() == 1 &&
isVerilogExpression(&op) &&
!mlir::MemoryEffectOpInterface::hasNoEffect(&op)) {
if (rewriteSideEffectingExpr(&op))
// If the target doesn't support local variables, hoist all the expressions
// out to the nearest non-procedural region.
if (options.disallowLocalVariables && isVerilogExpression(&op) &&
isProceduralRegion) {

// Force any side-effecting expressions in nested regions into a sv.reg
// if we aren't allowing local variable declarations. The Verilog emitter
// doesn't want to have to have to know how to synthesize a reg in the
// case they have to be spilled for whatever reason.
if (!mlir::MemoryEffectOpInterface::hasNoEffect(&op)) {
if (rewriteSideEffectingExpr(&op))
continue;
}

// Hoist other expressions out to the parent region.
//
// NOTE: This effectively disables inlining of expressions into if
// conditions, $fwrite statements, and instance inputs. We could be
// smarter in ExportVerilog itself, but we'd have to teach it to put
// spilled expressions (due to line length, multiple-uses, and
// non-inlinable expressions) in the outer scope.
if (hoistNonSideEffectExpr(&op))
continue;
}

// Duplicate "always inline" expression for each of their users and move
// them to be next to their users.
if (isExpressionAlwaysInline(&op)) {
lowerAlwaysInlineOperation(&op);
continue;
}
}

// Now that all the basic ops are settled, check for any use-before def issues
// in graph regions. Lower these into explicit wires to keep the emitter
// simple.
if (!block.getParentOp()->hasTrait<ProceduralRegion>()) {
if (!isProceduralRegion) {
SmallPtrSet<Operation *, 32> seenOperations;

for (auto &op : llvm::make_early_inc_range(block)) {
Expand Down
55 changes: 53 additions & 2 deletions test/ExportVerilog/disallow-local-vars.mlir
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,16 @@ hw.module @side_effect_expr(%clock: i1) -> (%a: i1, %a2: i1) {

// DISALLOW: reg [[SE_REG:[_A-Za-z0-9]+]];

// DISALLOW: wire [[COND:[_A-Za-z0-9]+]] = INLINE_OK;

// CHECK: always @(posedge clock)
// DISALLOW: always @(posedge clock)
sv.always posedge %clock {
%0 = sv.verbatim.expr "INLINE_OK" : () -> i1

// This shouldn't be touched.
// This shouldn't be pushed into a reg.
// CHECK: if (INLINE_OK)
// DISALLOW: if (INLINE_OK)
// DISALLOW: if ([[COND]])
sv.if %0 {
sv.fatal
}
Expand All @@ -42,3 +44,52 @@ hw.module @side_effect_expr(%clock: i1) -> (%a: i1, %a2: i1) {
// DISALLOW: assign a2 = YES_SE;
hw.output %2, %3: i1, i1
}

// CHECK-LABEL: module hoist_expressions
// DISALLOW-LABEL: module hoist_expressions
hw.module @hoist_expressions(%clock: i1, %x: i8, %y: i8, %z: i8) {
// DISALLOW: wire [7:0] [[ADD:[_A-Za-z0-9]+]] = x + y;
// DISALLOW: wire [[EQ:[_A-Za-z0-9]+]] = [[ADD]] == z;
// DISALLOW: wire [7:0] [[MUL:[_A-Za-z0-9]+]] = [[ADD]] * z;

// CHECK: always @(posedge clock)
// DISALLOW: always @(posedge clock)
sv.always posedge %clock {
%0 = comb.add %x, %y: i8
%1 = comb.icmp eq %0, %z : i8

// This shouldn't be touched.
// CHECK: if (_T == z) begin
// DISALLOW: if ([[EQ]]) begin
sv.if %1 {
// CHECK: $fwrite(32'h80000002, "Hi %x\n", _T * z);
// DISALLOW: $fwrite(32'h80000002, "Hi %x\n", [[MUL]]);
%2 = comb.mul %0, %z : i8
sv.fwrite "Hi %x\0A"(%2) : i8
sv.fatal
}
}

// Check out wires.
// CHECK: assign myWire = x;
// DISALLOW: assign myWire = x;
%myWire = sv.wire : !hw.inout<i8>
sv.assign %myWire, %x : i8

// DISALLOW: wire [[COND:[_A-Za-z0-9]+]] = x + myWire == z;

// CHECK: always @(posedge clock)
// DISALLOW: always @(posedge clock)
sv.always posedge %clock {
%wireout = sv.read_inout %myWire : !hw.inout<i8>
%3 = comb.add %x, %wireout: i8
%4 = comb.icmp eq %3, %z : i8
// CHECK: if (x + myWire == z)
// DISALLOW: if ([[COND]])
sv.if %4 {
sv.fatal
}
}

hw.output
}

0 comments on commit 05c7a13

Please sign in to comment.