From 8829a399e4d076392e2d31d9a6dfe8e500424912 Mon Sep 17 00:00:00 2001 From: Fabian Schuiki Date: Wed, 1 Mar 2023 17:53:30 -0800 Subject: [PATCH] [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 | 9 +-- .../ExportVerilog/ExportVerilog.cpp | 8 --- .../ExportVerilog/PrepareForEmission.cpp | 42 +++++++++++++ lib/Support/LoweringOptions.cpp | 8 +-- .../disallow-array-index-inlining.mlir | 23 ------- ...ate-vivado-array-index-const-prop-bug.mlir | 63 +++++++++++++++++++ 6 files changed, 114 insertions(+), 39 deletions(-) delete mode 100644 test/Conversion/ExportVerilog/disallow-array-index-inlining.mlir create mode 100644 test/Conversion/ExportVerilog/mitigate-vivado-array-index-const-prop-bug.mlir diff --git a/include/circt/Support/LoweringOptions.h b/include/circt/Support/LoweringOptions.h index 36d5f7acb5ff..f372fbd95257 100644 --- a/include/circt/Support/LoweringOptions.h +++ b/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. diff --git a/lib/Conversion/ExportVerilog/ExportVerilog.cpp b/lib/Conversion/ExportVerilog/ExportVerilog.cpp index e08463426beb..f0a02345ed64 100644 --- a/lib/Conversion/ExportVerilog/ExportVerilog.cpp +++ b/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(op)) - if (isa(user)) - if (op->getResult(0) == user->getOperand(1)) - return true; } return false; } diff --git a/lib/Conversion/ExportVerilog/PrepareForEmission.cpp b/lib/Conversion/ExportVerilog/PrepareForEmission.cpp index 940ff4adbdfe..f392cfaa0c40 100644 --- a/lib/Conversion/ExportVerilog/PrepareForEmission.cpp +++ b/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(&op)) { + + // Check if the index expression is already a wire. + Value wireOp; + Value readOp; + if (auto maybeReadOp = + op.getOperand(1).getDefiningOp()) { + if (isa_and_nonnull( + 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()) { + wireOp = builder.create(type, name); + builder.create(wireOp, op.getOperand(1)); + } else { + wireOp = builder.create(type, name); + builder.create(wireOp, op.getOperand(1)); + } + readOp = builder.create(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 diff --git a/lib/Support/LoweringOptions.cpp b/lib/Support/LoweringOptions.cpp index 5643250d2856..321125ff21ce 100644 --- a/lib/Support/LoweringOptions.cpp +++ b/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) + ','; diff --git a/test/Conversion/ExportVerilog/disallow-array-index-inlining.mlir b/test/Conversion/ExportVerilog/disallow-array-index-inlining.mlir deleted file mode 100644 index 0bba261b8584..000000000000 --- a/test/Conversion/ExportVerilog/disallow-array-index-inlining.mlir +++ /dev/null @@ -1,23 +0,0 @@ -// RUN: circt-opt --export-verilog %s | FileCheck %s -// RUN: circt-opt --test-apply-lowering-options='options=disallowArrayIndexInlining' --export-verilog %s | FileCheck %s --check-prefix=DISALLOW - -// CHECK-LABEL: module Foo( -// DISALLOW-LABEL: module Foo( -hw.module @Foo(%a: !hw.array<16xi1>, %b : i4) -> (x: i1, y: i1) { - // CHECK: assign x = a[b + 4'h1]; - // DISALLOW-DAG: wire [3:0] [[IDX0:.+]] = b + 4'h1; - // DISALLOW-DAG: assign x = 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 - - // CHECK: assign y = a[b * (b + 4'h2)]; - // DISALLOW-DAG: wire [3:0] [[IDX1:.+]] = b * (b + 4'h2); - // DISALLOW-DAG: assign y = a[[[IDX1]]]; - %c2_i4 = hw.constant 2 : i4 - %2 = comb.add %b, %c2_i4 : i4 - %3 = comb.mul %b, %2 : i4 - %4 = hw.array_get %a[%3] : !hw.array<16xi1>, i4 - - hw.output %1, %4 : i1, i1 -} diff --git a/test/Conversion/ExportVerilog/mitigate-vivado-array-index-const-prop-bug.mlir b/test/Conversion/ExportVerilog/mitigate-vivado-array-index-const-prop-bug.mlir new file mode 100644 index 000000000000..bae208ec8b68 --- /dev/null +++ b/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 + sv.assign %existingWire, %0 : i4 + %1 = sv.read_inout %existingWire : !hw.inout + %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