[PrepareForEmission] Add max expression size before creating a wire. (#…

…2795)

If an expression has more than the maximum number of terms, and we are
able to drop a wire or hoist it and drop a wire, we do so. The intent
is to remove the burden of spilling expressions larger than the
maximum token limit of some simulators by guessing what's "too large"
and doing it before ExportVerilog. Better algorithms and heuristics
for deciding to drop a wire can be added to shouldSpillWire.

The current implementation of prepareHWModule makes the phase ordering
of this a little interesting: we need to know it is "too large" before
we break the expression up into a binary tree. We also need to move
this addition, the splitting, and other canonicalizations to later in
prepareHWModule. If the spill happens while we are working on a
non-procedural region, we we can just proceed to splitting, but inside
a procedural region we actually move on down the block before
returning up.

include/circt/Support/LoweringOptions.h

@@ -86,6 +86,9 @@ struct LoweringOptions {
   enum { DEFAULT_TOKEN_NUMBER = 40000 };
   unsigned maximumNumberOfTokensPerExpression = DEFAULT_TOKEN_NUMBER;
 
+  enum { DEFAULT_TERM_LIMIT = 256 };
+  unsigned maximumNumberOfTermsPerExpression = DEFAULT_TERM_LIMIT;
+
   /// This is the target width of lines in an emitted Verilog source file in
   /// columns.
   enum { DEFAULT_LINE_LENGTH = 90 };

lib/Conversion/ExportVerilog/PrepareForEmission.cpp

@@ -46,6 +46,18 @@ bool ExportVerilog::isSimpleReadOrPort(Value v) {
   return isa<WireOp, RegOp>(readSrc);
 }
 
+// Check if the value is deemed worth spilling into a wire.
+static bool shouldSpillWire(Operation &op, const LoweringOptions &options) {
+  auto isAssign = [](Operation *op) { return isa<AssignOp>(op); };
+
+  // If there are more than the maximum number of terms in this single result
+  // expression, and it hasn't already been spilled, this should spill.
+  return isVerilogExpression(&op) &&
+         op.getNumOperands() > options.maximumNumberOfTermsPerExpression &&
+         op.getNumResults() == 1 &&
+         llvm::none_of(op.getResult(0).getUsers(), isAssign);
+}
+
 // Given an invisible instance, make sure all inputs are driven from
 // wires or ports.
 static void lowerBoundInstance(InstanceOp op) {
@@ -379,40 +391,6 @@ void ExportVerilog::prepareHWModule(Block &block,
        opIterator != e;) {
     auto &op = *opIterator++;
 
-    // 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;
-    }
-
-    // Turn a + -cst  ==> a - cst
-    if (auto addOp = dyn_cast<comb::AddOp>(op)) {
-      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);
-          opIterator = Block::iterator(firstOp);
-          continue;
-        }
-      }
-    }
-
     // 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
@@ -492,6 +470,59 @@ void ExportVerilog::prepareHWModule(Block &block,
       lowerAlwaysInlineOperation(&op);
       continue;
     }
+
+    // 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;
+        }
+      }
+    }
+
+    // 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;
+    }
+
+    // Turn a + -cst  ==> a - cst
+    if (auto addOp = dyn_cast<comb::AddOp>(op)) {
+      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);
+          opIterator = Block::iterator(firstOp);
+          continue;
+        }
+      }
+    }
   }
 
   // Now that all the basic ops are settled, check for any use-before def issues

lib/Support/LoweringOptions.cpp

@@ -61,6 +61,12 @@ void LoweringOptions::parse(StringRef text, ErrorHandlerT errorHandler) {
         errorHandler("expected integer source width");
         maximumNumberOfTokensPerExpression = DEFAULT_TOKEN_NUMBER;
       }
+    } else if (option.startswith("maximumNumberOfTermsPerExpression=")) {
+      option = option.drop_front(strlen("maximumNumberOfTermsPerExpression="));
+      if (option.getAsInteger(10, maximumNumberOfTermsPerExpression)) {
+        errorHandler("expected integer source width");
+        maximumNumberOfTermsPerExpression = DEFAULT_TERM_LIMIT;
+      }
     } else {
       errorHandler(llvm::Twine("unknown style option \'") + option + "\'");
       // We continue parsing options after a failure.
@@ -90,6 +96,9 @@ std::string LoweringOptions::toString() const {
   if (maximumNumberOfTokensPerExpression != DEFAULT_TOKEN_NUMBER)
     options += "maximumNumberOfTokensPerExpression=" +
                std::to_string(maximumNumberOfTokensPerExpression) + ',';
+  if (maximumNumberOfTermsPerExpression != DEFAULT_TERM_LIMIT)
+    options += "maximumNumberOfTermsPerExpression=" +
+               std::to_string(maximumNumberOfTermsPerExpression) + ',';
 
   // Remove a trailing comma if present.
   if (!options.empty()) {
@@ -142,7 +151,8 @@ struct LoweringCLOptions {
           "Style options.  Valid flags include: alwaysFF, "
           "noAlwaysComb, exprInEventControl, disallowPackedArrays, "
           "disallowLocalVariables, verifLabels, emittedLineLength=<n>, "
-          "maximumNumberOfTokensPerExpression=<n>"),
+          "maximumNumberOfTokensPerExpression=<n>, "
+          "maximumNumberOfTermsPerExpression=<n>"),
       llvm::cl::value_desc("option")};
 };
 } // namespace

test/Conversion/ExportVerilog/max-terms.mlir

@@ -0,0 +1,38 @@
+// RUN: circt-opt -lowering-options=maximumNumberOfTermsPerExpression=4 --export-verilog %s | FileCheck %s
+
+// CHECK-LABEL: module large_use_in_procedural
+hw.module @large_use_in_procedural(%clock: i1, %a: i1) {
+  // CHECK: wire [[GEN:.+]];
+  // CHECK: reg [[REG:.+]];
+
+  // CHECK: assign [[GEN]] = a + a + a + a + a;
+  // CHECK: always
+  sv.always {
+    sv.ifdef.procedural "FOO" {
+      // This expression should be hoisted and spilled.
+      %1 = comb.add %a, %a, %a, %a, %a : i1
+      // CHECK: if ([[GEN]])
+      sv.if %1 {
+        sv.exit
+      }
+      %2 = comb.add %a, %a, %a, %a : i1
+      // CHECK: if (a + a + a + a)
+      sv.if %2 {
+        sv.exit
+      }
+    }
+  }
+
+  %reg = sv.reg : !hw.inout<i1>
+  sv.alwaysff(posedge %clock) {
+    // CHECK: [[REG]] <= a;
+    sv.passign %reg, %a : i1
+    %0 = sv.read_inout %reg : !hw.inout<i1>
+    // This expression cannot be hoisted, even though it's over the limit.
+    %1 = comb.add %0, %0, %0, %0, %0 : i1
+    // CHECK: if ([[REG]] + [[REG]] + [[REG]] + [[REG]] + [[REG]])
+    sv.if %1 {
+      sv.exit
+    }
+  }
+}