Skip to content

Commit

Permalink
[FIRRTL][SV] Add comment attribute to GC interfaces and modules (#3529)…
Browse files Browse the repository at this point in the history
… (#3923)

We need to add a comment `// VCS coverage exclude_file"` to all Grand
Central modules and interfaces.  When we needed something like this
previousl, it was previously handled by adding a `comment` attribute
onto `hw.module` and handled in lower to HW.

To support these new targets, we have to:

1. Add a comment attribute to SV interfaces.  To accomplish this, I used
   an optional string attribute.  I switched the printed format to print
   the attribute dictionary after the symbol name, which I think is more
   common across all our operations.
2. Update ExportVerilog to print this attribute.  The `emitComment`
   function can recognize null attributes and skip a comment.
3. Propagate a "comment" attribute on FIRRTL modules in LowerToHW to HW
   modules.  This was not added to the ODS arguments, similar to the
   `output_file` attribute.
4. Modify GrandCentral to attach these comments to generated modules and
   interfaces.

I tested this on a design and there were no more files in the `scope`
directory missing this attribute.

Co-authored-by: Hideto Ueno <[email protected]>
  • Loading branch information
youngar and uenoku authored Sep 16, 2022
1 parent c88b624 commit 3066103
Show file tree
Hide file tree
Showing 8 changed files with 49 additions and 28 deletions.
5 changes: 3 additions & 2 deletions include/circt/Dialect/SV/SVTypeDecl.td
Original file line number Diff line number Diff line change
Expand Up @@ -37,12 +37,13 @@ def InterfaceOp : SVOp<"interface",
}];

let arguments = (ins
SymbolNameAttr:$sym_name
SymbolNameAttr:$sym_name,
OptionalAttr<StrAttr>:$comment
);

let regions = (region SizedRegion<1>:$body);

let assemblyFormat = "attr-dict $sym_name $body";
let assemblyFormat = "$sym_name attr-dict-with-keyword $body";

let skipDefaultBuilders = 1;
let builders = [
Expand Down
1 change: 1 addition & 0 deletions lib/Conversion/ExportVerilog/ExportVerilog.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3521,6 +3521,7 @@ LogicalResult StmtEmitter::visitSV(BindOp op) {
}

LogicalResult StmtEmitter::visitSV(InterfaceOp op) {
emitComment(op.commentAttr());
os << "interface " << getSymOpName(op) << ";\n";
// FIXME: Don't emit the body of this as general statements, they aren't!
emitStatementBlock(*op.getBodyBlock());
Expand Down
2 changes: 2 additions & 0 deletions lib/Conversion/FIRRTLToHW/LowerToHW.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1043,6 +1043,8 @@ FIRRTLModuleLowering::lowerModule(FModuleOp oldModule, Block *topLevelModule,
builder.create<hw::HWModuleOp>(oldModule.getLoc(), nameAttr, ports);
if (auto outputFile = oldModule->getAttr("output_file"))
newModule->setAttr("output_file", outputFile);
if (auto comment = oldModule->getAttrOfType<StringAttr>("comment"))
newModule.commentAttr(comment);

// If the circuit has an entry point, set all other modules private.
// Otherwise, mark all modules as public.
Expand Down
5 changes: 5 additions & 0 deletions lib/Dialect/FIRRTL/Transforms/GrandCentral.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1408,6 +1408,7 @@ GrandCentralPass::traverseBundle(AugmentedBundleTypeAttr bundle, IntegerAttr id,
&getContext(), getOutputDirectory().getValue(),
iFaceName + ".sv",
/*excludFromFileList=*/true));
iface.commentAttr(builder.getStringAttr("VCS coverage exclude_file"));

builder.setInsertionPointToEnd(cast<sv::InterfaceOp>(iface).getBody());

Expand Down Expand Up @@ -1856,6 +1857,8 @@ void GrandCentralPass::runOnOperation() {
&getContext(), getOutputDirectory().getValue(),
mapping.getName() + ".sv",
/*excludeFromFilelist=*/true));
mapping->setAttr("comment", builder.getStringAttr(
"VCS coverage exclude_file"));
companionIDMap[id] = {name.getValue(), op, mapping};

// Instantiate the mapping module inside the companion. Keep the
Expand Down Expand Up @@ -1896,6 +1899,8 @@ void GrandCentralPass::runOnOperation() {
op.getName() + ".sv",
/*excludeFromFileList=*/true,
/*includeReplicatedOps=*/true));
op->setAttr("comment",
builder.getStringAttr("VCS coverage exclude_file"));

// Look for any blackboxes instantiated by the companion and mark
// them for inclusion in the Grand Central extraction directory.
Expand Down
2 changes: 2 additions & 0 deletions lib/Dialect/FIRRTL/Transforms/GrandCentralTaps.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -773,6 +773,8 @@ void GrandCentralTapsPass::runOnOperation() {
hw::OutputFileAttr::getFromDirectoryAndFilename(
&getContext(), maybeExtractDirectory.getValue(),
impl.getName() + ".sv"));
impl->setAttr("comment",
builder.getStringAttr("VCS coverage exclude_file"));
builder.setInsertionPointToEnd(impl.getBody());

// Connect the output ports to the appropriate tapped object.
Expand Down
4 changes: 4 additions & 0 deletions test/Conversion/ExportVerilog/sv-interfaces.mlir
Original file line number Diff line number Diff line change
Expand Up @@ -136,4 +136,8 @@ module {
// CHECK-NEXT: struct packed {logic repeat_0; } data;
}

// CHECK-LABEL: // interface with a comment
// CHECK-NEXT: interface interfaceWithComment
sv.interface @interfaceWithComment
attributes {comment = "interface with a comment"} {}
}
8 changes: 8 additions & 0 deletions test/Conversion/FIRRTLToHW/lower-to-hw.mlir
Original file line number Diff line number Diff line change
Expand Up @@ -1648,4 +1648,12 @@ firrtl.circuit "Simple" attributes {annotations = [{class =
firrtl.module private @eliminateSingleOutputConnects(in %a: !firrtl.uint<1>, out %b: !firrtl.uint<1>) {
firrtl.strictconnect %b, %a : !firrtl.uint<1>
}

// Check that modules with comments are lowered.
// CHECK-LABEL: hw.module private @Commented() attributes {
// CHECK-SAME: comment = "this module is commented"
// CHECK-SAME: }
firrtl.module private @Commented() attributes {
comment = "this module is commented"
} {}
}
50 changes: 24 additions & 26 deletions test/Dialect/FIRRTL/grand-central.mlir
Original file line number Diff line number Diff line change
Expand Up @@ -88,9 +88,9 @@ firrtl.circuit "InterfaceGroundType" attributes {
// CHECK-SAME: @DUT
// CHECK-SAME: #hw.innerNameRef<@DUT::@c>]

// CHECK: sv.interface {
// CHECK: sv.interface @Foo
// CHECK-SAME: comment = "VCS coverage exclude_file"
// CHECK-SAME: output_file = #hw.output_file<"gct-dir/Foo.sv"
// CHECK-SAME: @Foo
// CHECK-NEXT: sv.verbatim "// description of foo"
// CHECK-NEXT: sv.interface.signal @foo : i2
// CHECK-NEXT: sv.verbatim "// multi\0A// line\0A// description\0A// of\0A// bar"
Expand Down Expand Up @@ -167,9 +167,9 @@ firrtl.circuit "InterfaceVectorType" attributes {
// CHECK: %a_1 = firrtl.regreset
// CHECK-SAME: annotations = [{a}]

// CHECK: sv.interface {
// CHECK: sv.interface @Foo
// CHECK-SAME: comment = "VCS coverage exclude_file"
// CHECK-SAME: output_file = #hw.output_file<"gct-dir/Foo.sv"
// CHECK-SAME: @Foo
// CHECK-NEXT: sv.verbatim "// description of foo"
// CHECK-NEXT: sv.interface.signal @foo : !hw.uarray<2xi1>

Expand Down Expand Up @@ -240,15 +240,15 @@ firrtl.circuit "InterfaceBundleType" attributes {
// CHECK: %y = firrtl.wire
// CHECK-SAME: annotations = [{a}]

// CHECK: sv.interface {
// CHECK: sv.interface @Foo
// CHECK-SAME: comment = "VCS coverage exclude_file"
// CHECK-SAME: output_file = #hw.output_file<"gct-dir/Foo.sv"
// CHECK-SAME: @Foo
// CHECK-NEXT: sv.verbatim "// description of Bar"
// CHECK-NEXT: Bar bar();

// CHECK: sv.interface {
// CHECK: sv.interface @Bar
// CHECK-SAME: comment = "VCS coverage exclude_file"
// CHECK-SAME: output_file = #hw.output_file<"gct-dir/Bar.sv"
// CHECK-SAME: @Bar
// CHECK-NEXT: sv.interface.signal @b : i2
// CHECK-NEXT: sv.interface.signal @a : i1

Expand Down Expand Up @@ -364,7 +364,7 @@ firrtl.circuit "VecOfVec" attributes {
// CHECK-NEXT: assign {{[{][{]0[}][}]}}.foo[0][1]
// CHECK-SAME: #hw.innerNameRef<@DUT::@__View_Foo__>

// CHECK: sv.interface {{.+}} @Foo
// CHECK: sv.interface @Foo
// CHECK: sv.interface.signal @foo : !hw.uarray<1xuarray<2xi3>>

// -----
Expand Down Expand Up @@ -423,9 +423,9 @@ firrtl.circuit "InterfaceNode" attributes {
// CHECK: firrtl.node
// CHECK-SAME: annotations = [{a}]

// CHECK: sv.interface {
// CHECK: sv.interface @Foo
// CHECK-SAME: comment = "VCS coverage exclude_file"
// CHECK-SAME: output_file = #hw.output_file<"gct-dir/Foo.sv"
// CHECK-SAME: @Foo
// CHECK-NEXT: sv.verbatim "// some expression"
// CHECK-NEXT: sv.interface.signal @foo : i2

Expand Down Expand Up @@ -480,9 +480,9 @@ firrtl.circuit "InterfacePort" attributes {
// CHECK: firrtl.module @DUT
// CHECK-SAME: %a: !firrtl.uint<4> sym @a [{a}]

// CHECK: sv.interface {
// CHECK: sv.interface @Foo
// CHECK-SAME: comment = "VCS coverage exclude_file"
// CHECK-SAME: output_file = #hw.output_file<"gct-dir/Foo.sv"
// CHECK-SAME: @Foo
// CHECK-NEXT: sv.verbatim "// description of foo"
// CHECK-NEXT: sv.interface.signal @foo : i4

Expand Down Expand Up @@ -535,9 +535,9 @@ firrtl.circuit "UnsupportedTypes" attributes {
// CHECK-NOT: class = "sifive.enterprise.grandcentral.AugmentedBundleType"
// CHECK-SAME: {

// CHECK: sv.interface {
// CHECK: sv.interface @Foo
// CHECK-SAME: comment = "VCS coverage exclude_file"
// CHECK-SAME: output_file = #hw.output_file<"gct-dir/Foo.sv"
// CHECK-SAME: @Foo
// CHECK-NEXT: sv.verbatim "// <unsupported string type> string;"
// CHECK-NEXT: sv.verbatim "// <unsupported boolean type> boolean;"
// CHECK-NEXT: sv.verbatim "// <unsupported integer type> integer;"
Expand Down Expand Up @@ -612,9 +612,9 @@ firrtl.circuit "BindInterfaceTest" attributes {
// CHECK-SAME: doNotPrint = true

// The interface is added.
// CHECK: sv.interface {
// CHECK: sv.interface @InterfaceName
// CHECK-SAME: comment = "VCS coverage exclude_file"
// CHECK-SAME: output_file = #hw.output_file<"gct-dir/InterfaceName.sv"
// CHECK-SAME: @InterfaceName
// CHECK-NEXT: sv.interface.signal @_a : i8

// -----
Expand Down Expand Up @@ -676,13 +676,13 @@ firrtl.circuit "MultipleGroundTypeInterfaces" attributes {
}
}

// CHECK: sv.interface {
// CHECK: sv.interface @Foo
// CHECK-SAME: comment = "VCS coverage exclude_file"
// CHECK-SAME: output_file = #hw.output_file<"gct-dir/Foo.sv"
// CHECK-SAME: @Foo

// CHECK: sv.interface {
// CHECK: sv.interface @Bar
// CHECK-SAME: comment = "VCS coverage exclude_file"
// CHECK-SAME: output_file = #hw.output_file<"gct-dir/Bar.sv"
// CHECK-SAME: @Bar

// -----

Expand Down Expand Up @@ -723,7 +723,7 @@ firrtl.circuit "PrefixInterfacesAnnotation"
// CHECK-NOT: sifive.enterprise.grandcentral.PrefixInterfacesAnnotation

// Interface "Foo" is prefixed.
// CHECK: sv.interface @PREFIX_Foo {
// CHECK: sv.interface @PREFIX_Foo
// Interface "Bar" is prefixed, but not its name.
// CHECK-NEXT: PREFIX_Bar bar()

Expand Down Expand Up @@ -819,8 +819,7 @@ firrtl.circuit "NestedInterfaceVectorTypes" attributes {annotations = [
// CHECK-SAME: #hw.innerNameRef<@DUT::@__View_Foo__>
// CHECK-SAME: @DUT
// CHECK-SAME: #hw.innerNameRef<@DUT::@b2>
// CHECK: sv.interface {
// CHECK-SAME: @Foo
// CHECK: sv.interface @Foo
// CHECK-NEXT: sv.verbatim "// description of bar"
// CHECK-NEXT: sv.interface.signal @bar : !hw.uarray<2xuarray<3xi1>>

Expand Down Expand Up @@ -883,8 +882,7 @@ firrtl.circuit "VerbatimTypesInVector" attributes {annotations = [
}

// CHECK-LABEL: firrtl.circuit "VerbatimTypesInVector"
// CHECK: sv.interface {
// CHECK-SAME: @Foo
// CHECK: sv.interface @Foo
// CHECK-NEXT: sv.verbatim "// description of bar"
// CHECK-NEXT: sv.verbatim "// <unsupported string type> bar[2][3];"

Expand Down

0 comments on commit 3066103

Please sign in to comment.