[PrepareForEmission, LoweringOptions] Unify LoweringOptions regarding…

… term sizes into maximumNumberOfTermsPerExpression (#4078)

This PR remove LoweringOptions `maximumNumberOfVariadicOperands` and `maximumNumberOfTermsInConcat`. Originally these parameters were used to limit the number of terms in specific expressions but `maximumNumberOfTermsPerExpression` should be sufficient for that purpose.

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 };

include/circt/Support/LoweringOptionsParser.h

@@ -44,8 +44,7 @@ struct LoweringOptionsOption
                 "noAlwaysComb, exprInEventControl, disallowPackedArrays, "
                 "disallowLocalVariables, verifLabels, emittedLineLength=<n>, "
                 "maximumNumberOfTermsPerExpression=<n>, "
-                "maximumNumberOfTermsInConcat=<n>, explicitBitcast, "
-                "maximumNumberOfVariadicOperands=<n>, "
+                "explicitBitcast, "
                 "emitReplicatedOpsToHeader, "
                 "locationInfoStyle={plain,wrapInAtSquareBracket,none}, "
                 "disallowPortDeclSharing, printDebugInfo, useOldEmissionMode, "

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<ConcatOp>(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<Value, size_t> &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<Operation *> &newOps,
-                                     DenseMap<Value, size_t> &operandMap,
-                                     const LoweringOptions &options) {
+                                     SmallVector<Operation *> &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,26 +290,20 @@ 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<Value, size_t> &operandMap) {
+static Operation *rewriteAddWithNegativeConstant(comb::AddOp add,
+                                                 hw::ConstantOp rhsCst) {
   ImplicitLocOpBuilder builder(add.getLoc(), add);
 
   // Get the positive constant.
   auto negCst = builder.create<hw::ConstantOp>(-rhsCst.getValue());
   auto sub =
       builder.create<comb::SubOp>(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<ProceduralRegion>())
     return;
 
-  // TODO: Refactor `lowerUsersToTemporaryWire` to remove operandMap.
-  DenseMap<Value, size_t> 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<Value, size_t> 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<ProceduralRegion>())
-              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<Operation *> 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<hw::ConstantOp>()) {
         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);
   }
 }

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()) {

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) (

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]];