[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 = <expression>`, 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 <[email protected]>

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<mlir::ModuleOp> importFIRFile(llvm::SourceMgr &sourceMgr,

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<NodeOp>(initializer.getType(), initializer, id,
+ auto isNamed =
+ !getConstants().options.disableNamePreservation && ::isNamed(id);
+ auto result = builder.create<NodeOp>(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<NodeOp>(
+ 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<WireOp>(type, id, annotations, sym);
+ auto isNamed =
+ !getConstants().options.disableNamePreservation && ::isNamed(id);
+ auto result = builder.create<WireOp>(
+ 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<NodeOp>(
+ type, result, id, getConstants().emptyArrayAttr,
+ StringAttr::get(annotations.getContext(), modNameSpace.newName(id)));
+ else {
+ auto debug = builder.create<WireOp>(
+ type.getPassiveType(), id, getConstants().emptyArrayAttr,
+ StringAttr::get(annotations.getContext(), modNameSpace.newName(id)));
+ builder.create<ConnectOp>(debug, result);
+ }
+ }
  return moduleContext.addSymbolEntry(id, result, startTok.getLoc());
 }
 

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 :

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 : %[[{

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:
 

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
 

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

tools/firtool/firtool.cpp

@@ -102,6 +102,11 @@ static cl::opt<bool> disableAnnotationsUnknown(
  "disable-annotation-unknown",
  cl::desc("Ignore unknown annotations when parsing"), cl::init(false));
 
+static cl::opt<bool> disableNamePreservation(
+ "disable-name-preservation",
+ cl::desc("Don't generate debug taps for named FIRRTL wires and nodes"),
+ cl::init(false));
+
 static cl::opt<bool>
  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");