[ExportVerilog][PrettifyVerilog] Fix exprInEventControl (#4625)

This commit fixes bugs in spilling logic regarding expressions used in sensitivity list.

* PrettifyVerilog is changed to check lowering options so that CSEd expressions are not cloned into users when `exprInEventControl` is enabled.
* There was a duplicated logic in isExpressionUnableToInline and PrepareForEmission so the logic was unified into isExpressionUnableToInline.

Fix https://github.com/llvm/circt/issues/4620

lib/Conversion/ExportVerilog/ExportVerilog.cpp

@@ -585,7 +585,8 @@ static bool isOkToBitSelectFrom(Value v) {
 /// happens because not all Verilog expressions are composable, notably you
 /// can only use bit selects like x[4:6] on simple expressions, you cannot use
 /// expressions in the sensitivity list of always blocks, etc.
-static bool isExpressionUnableToInline(Operation *op) {
+static bool isExpressionUnableToInline(Operation *op,
+                                       const LoweringOptions &options) {
   if (auto cast = dyn_cast<BitcastOp>(op))
     if (!haveMatchingDims(cast.getInput().getType(), cast.getResult().getType(),
                           op->getLoc())) {
@@ -629,7 +630,7 @@ static bool isExpressionUnableToInline(Operation *op) {
         return true;
 
     // Always blocks must have a name in their sensitivity list, not an expr.
-    if (isa<AlwaysOp>(user) || isa<AlwaysFFOp>(user)) {
+    if (!options.allowExprInEventControl && isa<AlwaysOp, AlwaysFFOp>(user)) {
       // Anything other than a read of a wire must be out of line.
       if (auto read = dyn_cast<ReadInOutOp>(op))
         if (read.getInput().getDefiningOp<WireOp>() ||
@@ -715,7 +716,7 @@ bool ExportVerilog::isExpressionEmittedInline(Operation *op,
 
   // If it isn't structurally possible to inline this expression, emit it out
   // of line.
-  return !isExpressionUnableToInline(op);
+  return !isExpressionUnableToInline(op, options);
 }
 
 /// Find a nested IfOp in an else block that can be printed as `else if`

lib/Conversion/ExportVerilog/PrepareForEmission.cpp

@@ -723,42 +723,6 @@ static LogicalResult legalizeHWModule(Block &block,
       }
     }
 
-    // Force any expression used in the event control of an always process to be
-    // a trivial wire, if the corresponding option is set.
-    if (!options.allowExprInEventControl) {
-      auto enforceWire = [&](Value expr) {
-        // Direct port uses are fine.
-        if (isSimpleReadOrPort(expr))
-          return;
-        if (auto inst = expr.getDefiningOp<InstanceOp>())
-          return;
-        auto builder = ImplicitLocOpBuilder::atBlockBegin(
-            op.getLoc(), &op.getParentOfType<HWModuleOp>().front());
-        auto newWire = builder.create<WireOp>(expr.getType());
-        builder.setInsertionPoint(&op);
-        auto newWireRead = builder.create<ReadInOutOp>(newWire);
-        // For simplicity, replace all uses with the read first.  This lets us
-        // recursive root out all uses of the expression.
-        expr.replaceAllUsesWith(newWireRead);
-        builder.setInsertionPoint(&op);
-        builder.create<AssignOp>(newWire, expr);
-        // To get the output correct, given that reads are always inline,
-        // duplicate them for each use.
-        lowerAlwaysInlineOperation(newWireRead);
-      };
-      if (auto always = dyn_cast<AlwaysOp>(op)) {
-        for (auto clock : always.getClocks())
-          enforceWire(clock);
-        continue;
-      }
-      if (auto always = dyn_cast<AlwaysFFOp>(op)) {
-        enforceWire(always.getClock());
-        if (auto reset = always.getReset())
-          enforceWire(reset);
-        continue;
-      }
-    }
-
     // If the target doesn't support local variables, hoist all the expressions
     // out to the nearest non-procedural region.
     if (options.disallowLocalVariables && isVerilogExpression(&op) &&

lib/Dialect/SV/Transforms/PrettifyVerilog.cpp

@@ -22,6 +22,7 @@
 #include "circt/Dialect/Comb/CombOps.h"
 #include "circt/Dialect/HW/HWOps.h"
 #include "circt/Dialect/SV/SVPasses.h"
+#include "circt/Support/LoweringOptions.h"
 #include "mlir/IR/ImplicitLocOpBuilder.h"
 #include "mlir/IR/Matchers.h"
 #include "llvm/ADT/TypeSwitch.h"
@@ -51,6 +52,7 @@ private:
   bool splitAssignment(OpBuilder &builder, Value dst, Value src);
 
   bool anythingChanged;
+  LoweringOptions options;
 
   DenseSet<Operation *> toDelete;
 };
@@ -337,6 +339,9 @@ bool PrettifyVerilogPass::prettifyUnaryOperator(Operation *op) {
   for (auto *user : op->getUsers()) {
     if (isa<comb::ExtractOp, hw::ArraySliceOp>(user))
       return false;
+    if (!options.allowExprInEventControl &&
+        isa<sv::AlwaysFFOp, sv::AlwaysOp>(user))
+      return false;
   }
 
   // Duplicating unary operations can move them across blocks (down the region
@@ -531,6 +536,7 @@ void PrettifyVerilogPass::processPostOrder(Block &body) {
 
 void PrettifyVerilogPass::runOnOperation() {
   hw::HWModuleOp thisModule = getOperation();
+  options = LoweringOptions(thisModule->getParentOfType<mlir::ModuleOp>());
 
   // Keeps track if anything changed during this pass, used to determine if
   // the analyses were preserved.

test/Conversion/ExportVerilog/sv-always-wire.mlir

@@ -1,4 +1,5 @@
-// RUN: circt-opt %s --export-verilog --verify-diagnostics -o %t | FileCheck %s --strict-whitespace
+// RUN: circt-opt %s -prettify-verilog --export-verilog --verify-diagnostics -o %t | FileCheck %s --strict-whitespace
+// RUN: circt-opt %s -test-apply-lowering-options='options=exprInEventControl' -prettify-verilog -export-verilog | FileCheck %s --check-prefix=INLINE
 
 // CHECK-LABEL: module AlwaysSpill(
 hw.module @AlwaysSpill(%port: i1) {
@@ -23,17 +24,33 @@ hw.module @AlwaysSpill(%port: i1) {
 
   // Constant values should cause a spill.
   // CHECK: always @(posedge [[TMP1]])
+  // INLINE: always @(posedge 1'h0)
   sv.always posedge %false {}
   // CHECK: always_ff @(posedge [[TMP2]])
+  // INLINE: always_ff @(posedge 1'h1)
   sv.alwaysff(posedge %true) {}
 }
 
 // CHECK-LABEL: module Foo
-hw.module @Foo(%clock: i1, %reset0: i1, %reset1: i1) -> () {
+// INLINE-LABEL: module Foo
+hw.module @Foo(%reset0: i1, %reset1: i1) -> () {
   %0 = comb.or %reset0, %reset1 : i1
-// CHECK-NOT: _GEN_0
+  // CHECK:      wire [[TMP0:.*]] = reset0 | reset1;
+  // CHECK-NEXT: always @(posedge [[TMP0]])
+  // CHECK-NEXT:   if ([[TMP0]])
   sv.always posedge %0 {
     sv.if %0 {
     }
   }
+  %true = hw.constant true
+  %1 = comb.xor %reset0, %true : i1
+  // CHECK:      wire [[TMP1:.*]] = ~reset0;
+  // CHECK-NEXT: always @(posedge [[TMP1]])
+  // CHECK-NEXT:   if ([[TMP1]])
+  // INLINE:     always @(posedge ~reset0)
+  // INLINE-NEXT:   if (~reset0)
+  sv.always posedge %1 {
+    sv.if %1 {
+    }
+  }
 }

test/Conversion/ExportVerilog/sv-dialect.mlir

@@ -1,4 +1,4 @@
-// RUN: circt-opt %s -test-apply-lowering-options='options=exprInEventControl,explicitBitcast,maximumNumberOfTermsPerExpression=10' -export-verilog -verify-diagnostics | FileCheck %s
+// RUN: circt-opt %s -test-apply-lowering-options='options=explicitBitcast,maximumNumberOfTermsPerExpression=10' -export-verilog -verify-diagnostics | FileCheck %s
 
 // CHECK-LABEL: module M1
 // CHECK-NEXT:    #(parameter [41:0] param1) (