[ExportVerilog] Add KEEP attr to Vivado array index bug workaround (#…

…4744)

Rename the `disallowArrayIndexInlining` lowering option to
`mitigateVivadoArrayIndexConstPropBug` to more clearly convey the fact
that this is intended as a Vivado bug workaround.

Since this is now Vivado-specific, add the `(* keep = "true" *)` SV
attribute to the array index wire, which is necessary to properly
mitigate the Vivado synthesis issue we were observing.

include/circt/Support/LoweringOptions.h

@@ -138,10 +138,11 @@ struct LoweringOptions {
   /// option avoids such warnings.
   bool disallowExpressionInliningInPorts = false;
 
-  /// If true, every expression used as an array index is driven by a wire. Some
-  /// tools, notably Vivado, produce incorrect synthesis results for certain
-  /// arithmetic ops inlined into the array index.
-  bool disallowArrayIndexInlining = false;
+  /// If true, every expression used as an array index is driven by a wire, and
+  /// the wire is marked as `(* keep = "true" *)`. Certain versions of Vivado
+  /// produce incorrect synthesis results for certain arithmetic ops inlined
+  /// into the array index.
+  bool mitigateVivadoArrayIndexConstPropBug = false;
 
   /// If true, emit `wire` in port lists rather than nothing. Used in cases
   /// where `default_nettype is not set to wire.

lib/Conversion/ExportVerilog/ExportVerilog.cpp

@@ -638,14 +638,6 @@ static bool isExpressionUnableToInline(Operation *op,
           continue;
       return true;
     }
-
-    // Force array index expressions to be a simple name when the
-    // `disallowArrayIndexInlining` option is set.
-    if (options.disallowArrayIndexInlining && !isa<sv::ReadInOutOp>(op))
-      if (isa<ArraySliceOp, ArrayGetOp, ArrayIndexInOutOp,
-              IndexedPartSelectInOutOp, IndexedPartSelectOp>(user))
-        if (op->getResult(0) == user->getOperand(1))
-          return true;
   }
   return false;
 }

lib/Conversion/ExportVerilog/PrepareForEmission.cpp

@@ -764,6 +764,48 @@ static LogicalResult legalizeHWModule(Block &block,
       continue;
     }
 
+    // Force array index expressions to be a simple name when the
+    // `mitigateVivadoArrayIndexConstPropBug` option is set.
+    if (options.mitigateVivadoArrayIndexConstPropBug &&
+        isa<ArraySliceOp, ArrayGetOp, ArrayIndexInOutOp,
+            IndexedPartSelectInOutOp, IndexedPartSelectOp>(&op)) {
+
+      // Check if the index expression is already a wire.
+      Value wireOp;
+      Value readOp;
+      if (auto maybeReadOp =
+              op.getOperand(1).getDefiningOp<sv::ReadInOutOp>()) {
+        if (isa_and_nonnull<WireOp, LogicOp>(
+                maybeReadOp.getInput().getDefiningOp())) {
+          wireOp = maybeReadOp.getInput();
+          readOp = maybeReadOp;
+        }
+      }
+
+      // Insert a wire and read if necessary.
+      ImplicitLocOpBuilder builder(op.getLoc(), &op);
+      if (!wireOp) {
+        auto type = op.getOperand(1).getType();
+        const auto *name = "_GEN_ARRAY_IDX";
+        if (op.getParentOp()->hasTrait<ProceduralRegion>()) {
+          wireOp = builder.create<LogicOp>(type, name);
+          builder.create<BPAssignOp>(wireOp, op.getOperand(1));
+        } else {
+          wireOp = builder.create<WireOp>(type, name);
+          builder.create<AssignOp>(wireOp, op.getOperand(1));
+        }
+        readOp = builder.create<ReadInOutOp>(wireOp);
+      }
+      op.setOperand(1, readOp);
+
+      // Add a `(* keep = "true" *)` SV attribute to the wire.
+      sv::addSVAttributes(wireOp.getDefiningOp(),
+                          SVAttributeAttr::get(wireOp.getContext(), "keep",
+                                               R"("true")",
+                                               /*emitAsComment=*/false));
+      continue;
+    }
+
     // If this expression is deemed worth spilling into a wire, do it here.
     if (shouldSpillWire(op, options)) {
       // We first check that it is possible to reuse existing wires as a spilled

lib/Support/LoweringOptions.cpp

@@ -95,8 +95,8 @@ void LoweringOptions::parse(StringRef text, ErrorHandlerT errorHandler) {
       disallowExpressionInliningInPorts = true;
     } else if (option == "disallowMuxInlining") {
       disallowMuxInlining = true;
-    } else if (option == "disallowArrayIndexInlining") {
-      disallowArrayIndexInlining = true;
+    } else if (option == "mitigateVivadoArrayIndexConstPropBug") {
+      mitigateVivadoArrayIndexConstPropBug = true;
     } else if (option.consume_front("wireSpillingHeuristic=")) {
       if (auto heuristic = parseWireSpillingHeuristic(option)) {
         wireSpillingHeuristicSet |= *heuristic;
@@ -150,8 +150,8 @@ std::string LoweringOptions::toString() const {
     options += "disallowExpressionInliningInPorts,";
   if (disallowMuxInlining)
     options += "disallowMuxInlining,";
-  if (disallowArrayIndexInlining)
-    options += "disallowArrayIndexInlining,";
+  if (mitigateVivadoArrayIndexConstPropBug)
+    options += "mitigateVivadoArrayIndexConstPropBug,";
 
   if (emittedLineLength != DEFAULT_LINE_LENGTH)
     options += "emittedLineLength=" + std::to_string(emittedLineLength) + ',';

test/Conversion/ExportVerilog/mitigate-vivado-array-index-const-prop-bug.mlir

@@ -0,0 +1,63 @@
+// RUN: circt-opt --export-verilog %s | FileCheck %s
+// RUN: circt-opt --test-apply-lowering-options='options=mitigateVivadoArrayIndexConstPropBug' --export-verilog %s | FileCheck %s --check-prefix=FIXED
+
+// CHECK-LABEL: module Simple(
+// FIXED-LABEL: module Simple(
+hw.module @Simple(%a: !hw.array<16xi1>, %b : i4) -> (c: i1) {
+  // CHECK: assign c = a[b + 4'h1];
+
+  // FIXED:      (* keep = "true" *)
+  // FIXED-NEXT: wire [3:0] [[IDX0:.+]] = b + 4'h1;
+  // FIXED-NEXT: assign c = a[[[IDX0]]];
+
+  %c1_i4 = hw.constant 1 : i4
+  %0 = comb.add %b, %c1_i4 : i4
+  %1 = hw.array_get %a[%0] : !hw.array<16xi1>, i4
+  hw.output %1 : i1
+}
+// CHECK: endmodule
+// FIXED: endmodule
+
+
+// CHECK-LABEL: module ExistingWire(
+// FIXED-LABEL: module ExistingWire(
+hw.module @ExistingWire(%a: !hw.array<16xi1>, %b : i4) -> (c: i1) {
+  // CHECK:      wire [3:0] existingWire = b + 4'h3;
+  // CHECK-NEXT: assign c = a[existingWire];
+
+  // FIXED:      (* keep = "true" *)
+  // FIXED-NEXT: wire [3:0] existingWire = b + 4'h3;
+  // FIXED-NEXT: assign c = a[existingWire];
+
+  %c1_i4 = hw.constant 3 : i4
+  %0 = comb.add %b, %c1_i4 : i4
+  %existingWire = sv.wire : !hw.inout<i4>
+  sv.assign %existingWire, %0 : i4
+  %1 = sv.read_inout %existingWire : !hw.inout<i4>
+  %2 = hw.array_get %a[%1] : !hw.array<16xi1>, i4
+  hw.output %2 : i1
+}
+// CHECK: endmodule
+// FIXED: endmodule
+
+
+// CHECK-LABEL: module ProceduralRegion(
+// FIXED-LABEL: module ProceduralRegion(
+hw.module @ProceduralRegion(%a: !hw.array<16xi1>, %b : i4) {
+  // CHECK:      magic(a[b + 4'h1]);
+
+  // FIXED:      initial begin
+  // FIXED-NEXT:   (* keep = "true" *)
+  // FIXED-NEXT:   automatic logic [3:0] [[IDX0:.+]] = b + 4'h1;
+  // FIXED-NEXT:   magic(a[[[IDX0]]]);
+  // FIXED-NEXT: end
+
+  %c1_i4 = hw.constant 1 : i4
+  %0 = comb.add %b, %c1_i4 : i4
+  sv.initial {
+    %1 = hw.array_get %a[%0] : !hw.array<16xi1>, i4
+    sv.verbatim "magic({{0}});" (%1): i1
+  }
+}
+// CHECK: endmodule
+// FIXED: endmodule