[ExportVerilog] Fix crash on sv.reg with initial value (#6689)

* Change isMovableDeclaration to allow operations to be lifted if all their operands are constant expressions. * Adapt lowerUsersToTemporaryWire to handle InOut values.
llvm · Feb 13, 2024 · b94d4b1 · b94d4b1
1 parent 017bf54
commit b94d4b1
Show file tree

Hide file tree

Showing 2 changed files with 73 additions and 10 deletions.
diff --git a/lib/Conversion/ExportVerilog/PrepareForEmission.cpp b/lib/Conversion/ExportVerilog/PrepareForEmission.cpp
@@ -163,25 +163,50 @@ static void lowerUsersToTemporaryWire(Operation &op,
 
   auto createWireForResult = [&](Value result, StringAttr name) {
     Value newWire;
+    Type wireElementType = result.getType();
+    bool isResultInOut = false;
+
+    // If the result already is an InOut, make sure to not wrap it again
+    if (auto inoutType = hw::type_dyn_cast<hw::InOutType>(result.getType())) {
+      wireElementType = inoutType.getElementType();
+      isResultInOut = true;
+    }
+
     // If the op is in a procedural region, use logic op.
     if (isProceduralRegion)
-      newWire = builder.create<LogicOp>(result.getType(), name);
+      newWire = builder.create<LogicOp>(wireElementType, name);
     else
-      newWire = builder.create<sv::WireOp>(result.getType(), name);
+      newWire = builder.create<sv::WireOp>(wireElementType, name);
 
+    // Replace all uses with newWire. Wrap in ReadInOutOp if required.
     while (!result.use_empty()) {
-      auto newWireRead = builder.create<ReadInOutOp>(newWire);
       OpOperand &use = *result.getUses().begin();
-      use.set(newWireRead);
-      newWireRead->moveBefore(use.getOwner());
+      if (isResultInOut) {
+        use.set(newWire);
+      } else {
+        auto newWireRead = builder.create<ReadInOutOp>(newWire);
+        use.set(newWireRead);
+        newWireRead->moveBefore(use.getOwner());
+      }
     }
 
+    // Assign the original result to the temporary wire.
     Operation *connect;
+    ReadInOutOp resultRead;
+
+    if (isResultInOut)
+      resultRead = builder.create<ReadInOutOp>(result);
+
     if (isProceduralRegion)
-      connect = builder.create<BPAssignOp>(newWire, result);
+      connect = builder.create<BPAssignOp>(
+          newWire, isResultInOut ? resultRead.getResult() : result);
     else
-      connect = builder.create<AssignOp>(newWire, result);
+      connect = builder.create<AssignOp>(
+          newWire, isResultInOut ? resultRead.getResult() : result);
+
     connect->moveAfter(&op);
+    if (resultRead)
+      resultRead->moveBefore(connect);
 
     // Move the temporary to the appropriate place.
     if (!emitWireAtBlockBegin) {
@@ -467,9 +492,15 @@ static bool hoistNonSideEffectExpr(Operation *op) {
 
 /// Check whether an op is a declaration that can be moved.
 static bool isMovableDeclaration(Operation *op) {
-  return op->getNumResults() == 1 &&
-         op->getResult(0).getType().isa<InOutType, sv::InterfaceType>() &&
-         op->getNumOperands() == 0;
+  if (op->getNumResults() != 1 ||
+      !op->getResult(0).getType().isa<InOutType, sv::InterfaceType>())
+    return false;
+
+  // If all operands (e.g. init value) are constant, it is safe to move
+  return llvm::all_of(op->getOperands(), [](Value operand) -> bool {
+    auto *defOp = operand.getDefiningOp();
+    return !!defOp && isConstantExpression(defOp);
+  });
 }
 
 //===----------------------------------------------------------------------===//

diff --git a/test/Conversion/ExportVerilog/prepare-for-emission.mlir b/test/Conversion/ExportVerilog/prepare-for-emission.mlir
@@ -292,3 +292,35 @@ module attributes {circt.loweringOptions = "disallowLocalVariables"} {
   }
   hw.hierpath @xmr [@Foo::@a]
 }
+
+
+// -----
+
+// CHECK-LABEL: @constantInitRegWithBackEdge
+hw.module @constantInitRegWithBackEdge() {
+  // CHECK: %reg = sv.reg init %false : !hw.inout<i1>
+  // CHECK-NEXT: %false = hw.constant false
+  // CHECK-NEXT: %[[VAL_0:.*]] = sv.read_inout %reg : !hw.inout<i1>
+  // CHECK-NEXT: %[[VAL_1:.*]] = comb.or %false, %[[VAL_0]] : i1
+  %false = hw.constant false
+  %0 = comb.or %false, %1 : i1
+  %reg = sv.reg init %false : !hw.inout<i1>
+  %1 = sv.read_inout %reg : !hw.inout<i1>
+}
+
+// -----
+
+// CHECK-LABEL: @temporaryWireForReg
+hw.module @temporaryWireForReg() {
+  // CHECK: %[[WIRE:.*]] = sv.wire : !hw.inout<i1>
+  // CHECK-NEXT: %[[VAL_0:.*]] = sv.read_inout %[[WIRE]]  : !hw.inout<i1>
+  // CHECK-NEXT: %b = sv.reg init %[[VAL_0]] : !hw.inout<i1>
+  // CHECK-NEXT: %[[VAL_1:.*]] = sv.read_inout %b : !hw.inout<i1>
+  // CHECK-NEXT: %a = sv.reg init %[[VAL_1]] : !hw.inout<i1>
+  // CHECK-NEXT: %[[VAL_2:.*]] = sv.read_inout %a : !hw.inout<i1>
+  // CHECK-NEXT: sv.assign %[[WIRE]], %[[VAL_2]] : i1
+  %0 = sv.read_inout %a : !hw.inout<i1>
+  %1 = sv.read_inout %b : !hw.inout<i1>
+  %b = sv.reg init %0 : !hw.inout<i1>
+  %a = sv.reg init %1 : !hw.inout<i1>
+}