diff --git a/include/circt/Support/LoweringOptions.h b/include/circt/Support/LoweringOptions.h index f44e0b1a5d6d..8f2ec397ee32 100644 --- a/include/circt/Support/LoweringOptions.h +++ b/include/circt/Support/LoweringOptions.h @@ -87,17 +87,6 @@ struct LoweringOptions { enum { DEFAULT_TERM_LIMIT = 256 }; unsigned maximumNumberOfTermsPerExpression = DEFAULT_TERM_LIMIT; - /// This is the maximum number of terms allow in a variadic expression before - /// it will spill to a wire. This is used to break up large product-of-sums - /// or sum-of-products for improved simulator performance. - enum { DEFAULT_VARIADIC_OPERAND_LIMIT = 32 }; - unsigned maximumNumberOfVariadicOperands = DEFAULT_VARIADIC_OPERAND_LIMIT; - - /// This is the maximum number of terms in an expression used in a concat - /// before that expression spills a wire. - enum { DEFAULT_CONCAT_TERM_LIMIT = 10 }; - unsigned maximumNumberOfTermsInConcat = DEFAULT_CONCAT_TERM_LIMIT; - /// This is the target width of lines in an emitted Verilog source file in /// columns. enum { DEFAULT_LINE_LENGTH = 90 }; diff --git a/include/circt/Support/LoweringOptionsParser.h b/include/circt/Support/LoweringOptionsParser.h index 733a6a1ea6de..bc4f4bfed801 100644 --- a/include/circt/Support/LoweringOptionsParser.h +++ b/include/circt/Support/LoweringOptionsParser.h @@ -44,8 +44,7 @@ struct LoweringOptionsOption "noAlwaysComb, exprInEventControl, disallowPackedArrays, " "disallowLocalVariables, verifLabels, emittedLineLength=, " "maximumNumberOfTermsPerExpression=, " - "maximumNumberOfTermsInConcat=, explicitBitcast, " - "maximumNumberOfVariadicOperands=, " + "explicitBitcast, " "emitReplicatedOpsToHeader, " "locationInfoStyle={plain,wrapInAtSquareBracket,none}, " "disallowPortDeclSharing, printDebugInfo, useOldEmissionMode, " diff --git a/lib/Conversion/ExportVerilog/PrepareForEmission.cpp b/lib/Conversion/ExportVerilog/PrepareForEmission.cpp index 20701e12c033..5ba76bc91848 100644 --- a/lib/Conversion/ExportVerilog/PrepareForEmission.cpp +++ b/lib/Conversion/ExportVerilog/PrepareForEmission.cpp @@ -54,8 +54,6 @@ bool ExportVerilog::isSimpleReadOrPort(Value v) { // Check if the value is deemed worth spilling into a wire. static bool shouldSpillWire(Operation &op, const LoweringOptions &options) { - auto isConcat = [](Operation *op) { return isa(op); }; - if (!isVerilogExpression(&op)) return false; @@ -65,17 +63,6 @@ static bool shouldSpillWire(Operation &op, const LoweringOptions &options) { !ExportVerilog::isExpressionEmittedInline(&op)) return true; - // Work around Verilator #3405. Large expressions inside a concat is worst - // case O(n^2) in a certain Verilator optimization, and can effectively hang - // Verilator on large designs. Verilator 4.224+ works around this by having a - // hard limit on its recursion. Here we break large expressions inside concats - // according to a configurable limit to work around the same issue. - // See https://github.com/verilator/verilator/issues/3405. - if (op.getNumOperands() > options.maximumNumberOfTermsInConcat && - op.getNumResults() == 1 && - llvm::any_of(op.getResult(0).getUsers(), isConcat)) - return true; - return false; } @@ -216,7 +203,6 @@ findLogicOpInsertionPoint(Operation *op) { /// a wire is created just after op's position so that we can inline the /// assignment into its wire declaration. static void lowerUsersToTemporaryWire(Operation &op, - DenseMap &operandMap, bool emitWireAtBlockBegin = false) { Block *block = op.getBlock(); auto builder = ImplicitLocOpBuilder::atBlockBegin(op.getLoc(), block); @@ -261,24 +247,19 @@ static void lowerUsersToTemporaryWire(Operation &op, auto namehint = inferStructuralNameForTemporary(op.getResult(0)); op.removeAttr("sv.namehint"); createWireForResult(op.getResult(0), namehint); - operandMap[op.getResult(0)] = 1; return; } // If the op has multiple results, create wires for each result. - for (auto result : op.getResults()) { + for (auto result : op.getResults()) createWireForResult(result, StringAttr()); - operandMap[result] = 1; - } } /// Lower a variadic fully-associative operation into an expression tree. This /// enables long-line splitting to work with them. /// NOLINTNEXTLINE(misc-no-recursion) static Value lowerFullyAssociativeOp(Operation &op, OperandRange operands, - SmallVector &newOps, - DenseMap &operandMap, - const LoweringOptions &options) { + SmallVector &newOps) { // save the top level name auto name = op.getAttr("sv.namehint"); if (name) @@ -296,21 +277,11 @@ static Value lowerFullyAssociativeOp(Operation &op, OperandRange operands, break; default: auto firstHalf = operands.size() / 2; - lhs = lowerFullyAssociativeOp(op, operands.take_front(firstHalf), newOps, - operandMap, options); - rhs = lowerFullyAssociativeOp(op, operands.drop_front(firstHalf), newOps, - operandMap, options); + lhs = lowerFullyAssociativeOp(op, operands.take_front(firstHalf), newOps); + rhs = lowerFullyAssociativeOp(op, operands.drop_front(firstHalf), newOps); break; } - if (operandMap[lhs] + operandMap[rhs] > - options.maximumNumberOfVariadicOperands) { - if (lhs.getDefiningOp()) - lowerUsersToTemporaryWire(*lhs.getDefiningOp(), operandMap); - if (rhs.getDefiningOp()) - lowerUsersToTemporaryWire(*rhs.getDefiningOp(), operandMap); - } - OperationState state(op.getLoc(), op.getName()); state.addOperands(ValueRange{lhs, rhs}); state.addTypes(op.getResult(0).getType()); @@ -319,18 +290,13 @@ static Value lowerFullyAssociativeOp(Operation &op, OperandRange operands, newOps.push_back(newOp); if (name) newOp->setAttr("sv.namehint", name); - size_t size = 0; - for (auto a : newOp->getOperands()) - size += operandMap[a]; - operandMap[newOp->getResult(0)] = size; return newOp->getResult(0); } /// Transform "a + -cst" ==> "a - cst" for prettier output. This returns the /// first operation emitted. -static Operation * -rewriteAddWithNegativeConstant(comb::AddOp add, hw::ConstantOp rhsCst, - DenseMap &operandMap) { +static Operation *rewriteAddWithNegativeConstant(comb::AddOp add, + hw::ConstantOp rhsCst) { ImplicitLocOpBuilder builder(add.getLoc(), add); // Get the positive constant. @@ -338,7 +304,6 @@ rewriteAddWithNegativeConstant(comb::AddOp add, hw::ConstantOp rhsCst, auto sub = builder.create(add.getOperand(0), negCst, add.getTwoState()); add.getResult().replaceAllUsesWith(sub); - operandMap.erase(add.getResult()); add.erase(); if (rhsCst.use_empty()) rhsCst.erase(); @@ -674,13 +639,11 @@ static void prettifyAfterLegalization( if (block.getParentOp()->hasTrait()) return; - // TODO: Refactor `lowerUsersToTemporaryWire` to remove operandMap. - DenseMap operandMap; for (auto &op : llvm::make_early_inc_range(block)) { if (!isVerilogExpression(&op)) continue; if (expressionStateManager.shouldSpillWireBasedOnState(op)) { - lowerUsersToTemporaryWire(op, operandMap); + lowerUsersToTemporaryWire(op); continue; } } @@ -698,10 +661,6 @@ static void prettifyAfterLegalization( /// otherwise rewriting operations we don't want to emit. static void legalizeHWModule(Block &block, const LoweringOptions &options) { - /// A cache of the number of operands that feed into an operation. This - /// avoids the need to look backwards across ops repeatedly. - DenseMap operandMap; - // First step, check any nested blocks that exist in this region. This walk // can pull things out to our level of the hierarchy. for (auto &op : block) { @@ -725,12 +684,6 @@ static void legalizeHWModule(Block &block, const LoweringOptions &options) { opIterator != e;) { auto &op = *opIterator++; - size_t totalOperands = 0; - for (auto a : op.getOperands()) - totalOperands += operandMap.count(a) ? operandMap[a] : 1; - if (op.getNumResults() == 1) - operandMap[op.getResult(0)] = totalOperands; - // Name legalization should have happened in a different pass for these sv // elements and we don't want to change their name through re-legalization // (e.g. letting a temporary take the name of an unvisited wire). Adding @@ -847,7 +800,7 @@ static void legalizeHWModule(Block &block, const LoweringOptions &options) { // If op is moved to a non-procedural region, create a temporary // wire. if (!op.getParentOp()->hasTrait()) - lowerUsersToTemporaryWire(op, operandMap); + 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 @@ -860,7 +813,7 @@ static void legalizeHWModule(Block &block, const LoweringOptions &options) { // If `disallowLocalVariables` is not enabled, we can spill the // expression to automatic logic declarations even when the op is in a // procedural region. - lowerUsersToTemporaryWire(op, operandMap); + lowerUsersToTemporaryWire(op); } } } @@ -878,9 +831,7 @@ static void legalizeHWModule(Block &block, const LoweringOptions &options) { (op.getAttrs().size() == 1 && op.hasAttr("sv.namehint")))) { // Lower this operation to a balanced binary tree of the same operation. SmallVector newOps; - auto result = lowerFullyAssociativeOp(op, op.getOperands(), newOps, - operandMap, options); - operandMap.erase(op.getResult(0)); + auto result = lowerFullyAssociativeOp(op, op.getOperands(), newOps); op.getResult(0).replaceAllUsesWith(result); op.erase(); @@ -894,8 +845,7 @@ static void legalizeHWModule(Block &block, const LoweringOptions &options) { if (auto cst = addOp.getOperand(1).getDefiningOp()) { assert(addOp.getNumOperands() == 2 && "commutative lowering is done"); if (cst.getValue().isNegative()) { - Operation *firstOp = - rewriteAddWithNegativeConstant(addOp, cst, operandMap); + Operation *firstOp = rewriteAddWithNegativeConstant(addOp, cst); opIterator = Block::iterator(firstOp); continue; } @@ -999,7 +949,7 @@ static void legalizeHWModule(Block &block, const LoweringOptions &options) { } // Otherwise, we need to lower this to a wire to resolve this. - lowerUsersToTemporaryWire(op, operandMap, + lowerUsersToTemporaryWire(op, /*emitWireAtBlockBegin=*/true); } } diff --git a/lib/Support/LoweringOptions.cpp b/lib/Support/LoweringOptions.cpp index 64788502e9f4..35f38385551f 100644 --- a/lib/Support/LoweringOptions.cpp +++ b/lib/Support/LoweringOptions.cpp @@ -82,11 +82,6 @@ void LoweringOptions::parse(StringRef text, ErrorHandlerT errorHandler) { errorHandler("expected integer source width"); maximumNumberOfTermsPerExpression = DEFAULT_TERM_LIMIT; } - } else if (option.consume_front("maximumNumberOfTermsInConcat=")) { - if (option.getAsInteger(10, maximumNumberOfTermsInConcat)) { - errorHandler("expected integer source width"); - maximumNumberOfTermsInConcat = DEFAULT_CONCAT_TERM_LIMIT; - } } else if (option.consume_front("locationInfoStyle=")) { if (auto style = parseLocationInfoStyle(option)) { locationInfoStyle = *style; @@ -101,11 +96,6 @@ void LoweringOptions::parse(StringRef text, ErrorHandlerT errorHandler) { useOldEmissionMode = true; } else if (option == "disallowExpressionInliningInPorts") { disallowExpressionInliningInPorts = true; - } else if (option.consume_front("maximumNumberOfVariadicOperands=")) { - if (option.getAsInteger(10, maximumNumberOfVariadicOperands)) { - errorHandler("expected integer for number of variadic operands"); - maximumNumberOfVariadicOperands = DEFAULT_VARIADIC_OPERAND_LIMIT; - } } else if (option.consume_front("wireSpillingHeuristic=")) { if (auto heuristic = parseWireSpillingHeuristic(option)) { wireSpillingHeuristicSet |= *heuristic; @@ -166,12 +156,6 @@ std::string LoweringOptions::toString() const { if (maximumNumberOfTermsPerExpression != DEFAULT_TERM_LIMIT) options += "maximumNumberOfTermsPerExpression=" + std::to_string(maximumNumberOfTermsPerExpression) + ','; - if (maximumNumberOfTermsInConcat != DEFAULT_CONCAT_TERM_LIMIT) - options += "maximumNumberOfTermsInConcat=" + - std::to_string(maximumNumberOfTermsInConcat) + ','; - if (maximumNumberOfVariadicOperands != DEFAULT_VARIADIC_OPERAND_LIMIT) - options += "maximumNumberOfVariadicOperands=" + - std::to_string(maximumNumberOfVariadicOperands) + ','; // Remove a trailing comma if present. if (!options.empty()) { diff --git a/test/Conversion/ExportVerilog/sv-dialect.mlir b/test/Conversion/ExportVerilog/sv-dialect.mlir index b20972d7d329..52620c4bc8dc 100644 --- a/test/Conversion/ExportVerilog/sv-dialect.mlir +++ b/test/Conversion/ExportVerilog/sv-dialect.mlir @@ -1,5 +1,5 @@ -// RUN: circt-opt %s -test-apply-lowering-options='options=exprInEventControl,explicitBitcast,maximumNumberOfTermsInConcat=10,useOldEmissionMode' -export-verilog -verify-diagnostics | FileCheck %s --strict-whitespace --check-prefixes=CHECK,OLD -// RUN: circt-opt %s -test-apply-lowering-options='options=exprInEventControl,explicitBitcast,maximumNumberOfTermsInConcat=10' -export-verilog -verify-diagnostics | FileCheck %s --check-prefixes=CHECK,NEW +// RUN: circt-opt %s -test-apply-lowering-options='options=exprInEventControl,explicitBitcast,maximumNumberOfTermsPerExpression=10,useOldEmissionMode' -export-verilog -verify-diagnostics | FileCheck %s --strict-whitespace --check-prefixes=CHECK,OLD +// RUN: circt-opt %s -test-apply-lowering-options='options=exprInEventControl,explicitBitcast,maximumNumberOfTermsPerExpression=10' -export-verilog -verify-diagnostics | FileCheck %s --check-prefixes=CHECK,NEW // CHECK-LABEL: module M1 // CHECK-NEXT: #(parameter [41:0] param1) ( diff --git a/test/Conversion/ExportVerilog/variadic-operand-splitting.mlir b/test/Conversion/ExportVerilog/variadic-operand-splitting.mlir index 009eea495956..466baf964c46 100644 --- a/test/Conversion/ExportVerilog/variadic-operand-splitting.mlir +++ b/test/Conversion/ExportVerilog/variadic-operand-splitting.mlir @@ -1,5 +1,5 @@ -// RUN: circt-opt -test-apply-lowering-options='options=maximumNumberOfVariadicOperands=8' -export-verilog %s | FileCheck %s -check-prefixes=CHECK,MAX_8 -// RUN: circt-opt -test-apply-lowering-options='options=maximumNumberOfVariadicOperands=4' -export-verilog %s | FileCheck %s -check-prefixes=CHECK,MAX_4 +// RUN: circt-opt -test-apply-lowering-options='options=maximumNumberOfTermsPerExpression=4' -export-verilog %s | FileCheck %s -check-prefixes=CHECK,MAX_8 +// RUN: circt-opt -test-apply-lowering-options='options=maximumNumberOfTermsPerExpression=2' -export-verilog %s | FileCheck %s -check-prefixes=CHECK,MAX_4 hw.module @Baz( %a0: i1, %a1: i1, %a2: i1, %a3: i1, @@ -21,12 +21,12 @@ hw.module @Baz( // CHECK-LABEL: module Baz -// MAX_8: wire [[wire_0:.+]] = a0 & b0 | a1 & b1 | a2 & b2 | a3 & b3; -// MAX_8: wire [[wire_1:.+]] = a4 & b4 | a5 & b5 | a6 & b6 | a7 & b7; -// MAX_8: assign c = [[wire_0]] | [[wire_1]]; +// MAX_8: wire [[wire_0:.+]] = a0 & b0 | a1 & b1 | a2 & b2 | a3 & b3; +// MAX_8-NEXT: wire [[wire_1:.+]] = a4 & b4 | a5 & b5 | a6 & b6 | a7 & b7; +// MAX_8-NEXT: assign c = [[wire_0]] | [[wire_1]]; -// MAX_4: wire [[wire_0:.+]] = a0 & b0 | a1 & b1; -// MAX_4: wire [[wire_1:.+]] = a2 & b2 | a3 & b3; -// MAX_4: wire [[wire_2:.+]] = a4 & b4 | a5 & b5; -// MAX_4: wire [[wire_3:.+]] = a6 & b6 | a7 & b7; -// MAX_4: assign c = [[wire_0]] | [[wire_1]] | [[wire_2]] | [[wire_3]]; +// MAX_4: wire [[wire_0:.+]] = a0 & b0 | a1 & b1; +// MAX_4-NEXT: wire [[wire_1:.+]] = a2 & b2 | a3 & b3; +// MAX_4-NEXT: wire [[wire_2:.+]] = a4 & b4 | a5 & b5; +// MAX_4-NEXT: wire [[wire_3:.+]] = a6 & b6 | a7 & b7; +// MAX_4-NEXT: assign c = [[wire_0]] | [[wire_1]] | [[wire_2]] | [[wire_3]];