From 106815aea4fe252f270af796a1b1762c77c15b0d Mon Sep 17 00:00:00 2001 From: Schuyler Eldridge Date: Fri, 25 Feb 2022 18:34:27 -0500 Subject: [PATCH] [FIRRTL] Preserve/Tap all "Named" Nodes or Wires (#2676) Change end-to-end FIRRTL compilation behavior to preserve (via tapping) all nodes and wires which are "named". A "named" node or wire is one whose name does not begin with an underscore. Tapping is done by creating a "shadow node" that is assigned the value of the actual wire and marked "don't touch". This is done to enable better debug-ability of Chisel designs by enabling users to always have references to named things they define in Chisel. More specifically, anytime a Chisel user defines a `val foo = `, CIRCT will now produce a wire called "foo" in the output Verilog. Add a parser option for controlling whether or not "named" wires and nodes will be preserved from FIRRTL to Verilog. Add a firtool command-line option for disabling name preservation during FIRRTL parsing. Signed-off-by: Schuyler Eldridge --- include/circt/Dialect/FIRRTL/FIRParser.h | 5 +++ lib/Dialect/FIRRTL/Import/FIRParser.cpp | 36 +++++++++++++++++- .../SFCTests/GrandCentralInterfaces/Wire.fir | 8 ++-- .../FIRRTL/SFCTests/data-taps-nlas.fir | 2 +- .../FIRRTL/SFCTests/signal-mappings.fir | 2 +- .../FIRRTL/SFCTests/width-spec-errors.fir | 2 +- test/firtool/name-preservation.fir | 37 +++++++++++++++++++ tools/firtool/firtool.cpp | 6 +++ 8 files changed, 89 insertions(+), 9 deletions(-) create mode 100644 test/firtool/name-preservation.fir diff --git a/include/circt/Dialect/FIRRTL/FIRParser.h b/include/circt/Dialect/FIRRTL/FIRParser.h index bbaf9efca50a..032ee9e65dd5 100644 --- a/include/circt/Dialect/FIRRTL/FIRParser.h +++ b/include/circt/Dialect/FIRRTL/FIRParser.h @@ -38,6 +38,11 @@ struct FIRParserOptions { /// This, along with numOMIRFiles provides structure to the buffers in the /// source manager. unsigned numAnnotationFiles; + /// If true, then the parser will NOT generate debug taps for "named" wires + /// and nodes. A "named" wire/node is one whose name does NOT beging with a + /// leading "_". If false, debug-ability is greatly increased. If true, much + /// more compact Verilog will be generated. + bool disableNamePreservation = true; }; mlir::OwningOpRef importFIRFile(llvm::SourceMgr &sourceMgr, diff --git a/lib/Dialect/FIRRTL/Import/FIRParser.cpp b/lib/Dialect/FIRRTL/Import/FIRParser.cpp index c9f7d415c8b2..8c7c7cb9cb01 100644 --- a/lib/Dialect/FIRRTL/Import/FIRParser.cpp +++ b/lib/Dialect/FIRRTL/Import/FIRParser.cpp @@ -2996,6 +2996,8 @@ ParseResult FIRStmtParser::parseMem(unsigned memIndent) { return moduleContext.addSymbolEntry(id, entryID, startTok.getLoc()); } +static bool isNamed(StringRef id) { return !id.startswith("_"); } + /// node ::= 'node' id '=' exp info? ParseResult FIRStmtParser::parseNode() { auto startTok = consumeToken(FIRToken::kw_node); @@ -3040,8 +3042,19 @@ ParseResult FIRStmtParser::parseNode() { moduleContext.targetsInModule, initializerType); auto sym = getSymbolIfRequired(annotations, id); - auto result = builder.create(initializer.getType(), initializer, id, + auto isNamed = + !getConstants().options.disableNamePreservation && ::isNamed(id); + auto result = builder.create(initializer.getType(), initializer, + isNamed ? (Twine("_") + id).str() : id, annotations, sym); + // If the node is named, then add a debug tap. + // + // TODO: Change this once the FIRRTL spec supports "named" vs. "unnamed" + // nodes. + if (isNamed) + builder.create( + initializer.getType(), result, id, getConstants().emptyArrayAttr, + StringAttr::get(annotations.getContext(), modNameSpace.newName(id))); return moduleContext.addSymbolEntry(id, result, startTok.getLoc()); } @@ -3070,7 +3083,26 @@ ParseResult FIRStmtParser::parseWire() { moduleContext.targetsInModule, type); auto sym = getSymbolIfRequired(annotations, id); - auto result = builder.create(type, id, annotations, sym); + auto isNamed = + !getConstants().options.disableNamePreservation && ::isNamed(id); + auto result = builder.create( + type, isNamed ? (Twine("_") + id).str() : id, annotations, sym); + // If the wire is named, then add a debug tap. + // + // TODO: Change this once the FIRRTL spec supports "named" vs. "unnamed" + // wires. + if (isNamed) { + if (type.isPassive()) + builder.create( + type, result, id, getConstants().emptyArrayAttr, + StringAttr::get(annotations.getContext(), modNameSpace.newName(id))); + else { + auto debug = builder.create( + type.getPassiveType(), id, getConstants().emptyArrayAttr, + StringAttr::get(annotations.getContext(), modNameSpace.newName(id))); + builder.create(debug, result); + } + } return moduleContext.addSymbolEntry(id, result, startTok.getLoc()); } diff --git a/test/Dialect/FIRRTL/SFCTests/GrandCentralInterfaces/Wire.fir b/test/Dialect/FIRRTL/SFCTests/GrandCentralInterfaces/Wire.fir index 916d713b9c78..e050434ee57d 100644 --- a/test/Dialect/FIRRTL/SFCTests/GrandCentralInterfaces/Wire.fir +++ b/test/Dialect/FIRRTL/SFCTests/GrandCentralInterfaces/Wire.fir @@ -1,7 +1,7 @@ -; RUN: firtool --firrtl-grand-central --verilog --annotation-file %S/Wire.anno.json --annotation-file %S/Extract.anno.json %s | FileCheck %s --check-prefixes CHECK,EXTRACT -; RUN: firtool --firrtl-grand-central --verilog --annotation-file %S/Wire.anno.json %s | FileCheck %s --check-prefixes CHECK,NOEXTRACT -; RUN: firtool --firrtl-grand-central --verilog --annotation-file %S/Wire.anno.json --annotation-file %S/Extract.anno.json %s --annotation-file %S/YAML.anno.json | FileCheck %s --check-prefixes YAML -; RUN: firtool --firrtl-grand-central --split-verilog --annotation-file %S/Wire.anno.json %s -o %t.folder > %t && cat %t.folder/MyView_companion.sv | FileCheck %s --check-prefixes MYVIEW_COMPANION +; RUN: firtool --firrtl-grand-central --disable-name-preservation --verilog --annotation-file %S/Wire.anno.json --annotation-file %S/Extract.anno.json %s | FileCheck %s --check-prefixes CHECK,EXTRACT +; RUN: firtool --firrtl-grand-central --disable-name-preservation --verilog --annotation-file %S/Wire.anno.json %s | FileCheck %s --check-prefixes CHECK,NOEXTRACT +; RUN: firtool --firrtl-grand-central --disable-name-preservation --verilog --annotation-file %S/Wire.anno.json --annotation-file %S/Extract.anno.json %s --annotation-file %S/YAML.anno.json | FileCheck %s --check-prefixes YAML +; RUN: firtool --firrtl-grand-central --disable-name-preservation --split-verilog --annotation-file %S/Wire.anno.json %s -o %t.folder > %t && cat %t.folder/MyView_companion.sv | FileCheck %s --check-prefixes MYVIEW_COMPANION circuit Top : module Submodule : diff --git a/test/Dialect/FIRRTL/SFCTests/data-taps-nlas.fir b/test/Dialect/FIRRTL/SFCTests/data-taps-nlas.fir index 22fac3ebfcd8..dca37772d90f 100644 --- a/test/Dialect/FIRRTL/SFCTests/data-taps-nlas.fir +++ b/test/Dialect/FIRRTL/SFCTests/data-taps-nlas.fir @@ -1,4 +1,4 @@ -; RUN: firtool --firrtl-grand-central --verilog %s | FileCheck %s +; RUN: firtool --firrtl-grand-central --disable-name-preservation --verilog %s | FileCheck %s ; See https://github.com/llvm/circt/issues/2691 circuit Top : %[[{ diff --git a/test/Dialect/FIRRTL/SFCTests/signal-mappings.fir b/test/Dialect/FIRRTL/SFCTests/signal-mappings.fir index 50a26a4237de..12814189bf56 100644 --- a/test/Dialect/FIRRTL/SFCTests/signal-mappings.fir +++ b/test/Dialect/FIRRTL/SFCTests/signal-mappings.fir @@ -1,4 +1,4 @@ -; RUN: firtool %s --annotation-file %S/signal-mappings-subCircuit.json --firrtl-grand-central --verilog | FileCheck %s +; RUN: firtool %s --annotation-file %S/signal-mappings-subCircuit.json --disable-name-preservation --firrtl-grand-central --verilog | FileCheck %s ; Subcircuit: diff --git a/test/Dialect/FIRRTL/SFCTests/width-spec-errors.fir b/test/Dialect/FIRRTL/SFCTests/width-spec-errors.fir index 32f61d7b58b4..fbbfd23cf362 100644 --- a/test/Dialect/FIRRTL/SFCTests/width-spec-errors.fir +++ b/test/Dialect/FIRRTL/SFCTests/width-spec-errors.fir @@ -1,4 +1,4 @@ -; RUN: firtool --split-input-file --verify-diagnostics %s +; RUN: firtool --disable-name-preservation --split-input-file --verify-diagnostics %s ; Tests extracted from: ; - test/scala/firrtlTests/WidthSpec.scala diff --git a/test/firtool/name-preservation.fir b/test/firtool/name-preservation.fir new file mode 100644 index 000000000000..20c804deddb1 --- /dev/null +++ b/test/firtool/name-preservation.fir @@ -0,0 +1,37 @@ +; RUN: firtool %s | FileCheck %s --check-prefixes=CHECK,NAMES +; RUN: firtool %s -disable-name-preservation | FileCheck %s --check-prefixes=CHECK,NO_NAMES + +circuit Foo: + ; CHECK-LABEL: module Foo + module Foo: + input a: {a: UInt<1>, flip b: UInt<1>} + output b: {a: UInt<1>, flip b: UInt<1>} + + ; Unnamed wires are always removed. + ; CHECK-NOT: wire _x_a; + ; CHECK-NOT: wire _x_b; + + wire _x: {a: UInt<1>, flip b: UInt<1>} + _x <= a + + ; Default behavior is to preserve named wires. + ; NAMES: wire x_a; + ; NAMES: wire x_b; + ; With -disable-name-preservation, named wires are removed. + ; NO_NAMES-NOT: wire x_b; + ; NO_NAMES-NOT: wire x_a; + wire x: {a: UInt<1>, flip b: UInt<1>} + x <= _x + + ; Unnamed nodes are always removed. + ; CHECK-NOT: wire _y_a; + node _y_a = x.a + + ; Default behavior is to preserve named nodes. + ; NAMES: wire y; + ; With -disable-name-preservation, named nodes are removed. + ; NO-NAMES-NOT: wire y; + node y = _y_a + + b.a <= y + x.b <= b.b diff --git a/tools/firtool/firtool.cpp b/tools/firtool/firtool.cpp index 80ba4dfa8a20..667a541d9fd9 100644 --- a/tools/firtool/firtool.cpp +++ b/tools/firtool/firtool.cpp @@ -102,6 +102,11 @@ static cl::opt disableAnnotationsUnknown( "disable-annotation-unknown", cl::desc("Ignore unknown annotations when parsing"), cl::init(false)); +static cl::opt disableNamePreservation( + "disable-name-preservation", + cl::desc("Don't generate debug taps for named FIRRTL wires and nodes"), + cl::init(false)); + static cl::opt emitMetadata("emit-metadata", cl::desc("emit metadata for metadata annotations"), @@ -381,6 +386,7 @@ processBuffer(MLIRContext &context, TimingScope &ts, llvm::SourceMgr &sourceMgr, options.ignoreInfoLocators = ignoreFIRLocations; options.rawAnnotations = newAnno; options.numAnnotationFiles = numAnnotationFiles; + options.disableNamePreservation = disableNamePreservation; module = importFIRFile(sourceMgr, &context, options); } else { auto parserTimer = ts.nest("MLIR Parser");