From 306610345b732e57ecdda39fa7e8d96fd0b3bdcf Mon Sep 17 00:00:00 2001 From: Andrew Young Date: Fri, 16 Sep 2022 16:06:37 -0700 Subject: [PATCH] [FIRRTL][SV] Add comment attribute to GC interfaces and modules (#3529) (#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 --- include/circt/Dialect/SV/SVTypeDecl.td | 5 +- .../ExportVerilog/ExportVerilog.cpp | 1 + lib/Conversion/FIRRTLToHW/LowerToHW.cpp | 2 + .../FIRRTL/Transforms/GrandCentral.cpp | 5 ++ .../FIRRTL/Transforms/GrandCentralTaps.cpp | 2 + .../ExportVerilog/sv-interfaces.mlir | 4 ++ test/Conversion/FIRRTLToHW/lower-to-hw.mlir | 8 +++ test/Dialect/FIRRTL/grand-central.mlir | 50 +++++++++---------- 8 files changed, 49 insertions(+), 28 deletions(-) diff --git a/include/circt/Dialect/SV/SVTypeDecl.td b/include/circt/Dialect/SV/SVTypeDecl.td index 131872a92bba..9380a187c72b 100644 --- a/include/circt/Dialect/SV/SVTypeDecl.td +++ b/include/circt/Dialect/SV/SVTypeDecl.td @@ -37,12 +37,13 @@ def InterfaceOp : SVOp<"interface", }]; let arguments = (ins - SymbolNameAttr:$sym_name + SymbolNameAttr:$sym_name, + OptionalAttr:$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 = [ diff --git a/lib/Conversion/ExportVerilog/ExportVerilog.cpp b/lib/Conversion/ExportVerilog/ExportVerilog.cpp index 6c3b57bafd42..722b0bd888d6 100644 --- a/lib/Conversion/ExportVerilog/ExportVerilog.cpp +++ b/lib/Conversion/ExportVerilog/ExportVerilog.cpp @@ -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()); diff --git a/lib/Conversion/FIRRTLToHW/LowerToHW.cpp b/lib/Conversion/FIRRTLToHW/LowerToHW.cpp index ab46013d2ac2..cc7f345d9b7a 100644 --- a/lib/Conversion/FIRRTLToHW/LowerToHW.cpp +++ b/lib/Conversion/FIRRTLToHW/LowerToHW.cpp @@ -1043,6 +1043,8 @@ FIRRTLModuleLowering::lowerModule(FModuleOp oldModule, Block *topLevelModule, builder.create(oldModule.getLoc(), nameAttr, ports); if (auto outputFile = oldModule->getAttr("output_file")) newModule->setAttr("output_file", outputFile); + if (auto comment = oldModule->getAttrOfType("comment")) + newModule.commentAttr(comment); // If the circuit has an entry point, set all other modules private. // Otherwise, mark all modules as public. diff --git a/lib/Dialect/FIRRTL/Transforms/GrandCentral.cpp b/lib/Dialect/FIRRTL/Transforms/GrandCentral.cpp index c213078016d5..60cb65fd9674 100644 --- a/lib/Dialect/FIRRTL/Transforms/GrandCentral.cpp +++ b/lib/Dialect/FIRRTL/Transforms/GrandCentral.cpp @@ -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(iface).getBody()); @@ -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 @@ -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. diff --git a/lib/Dialect/FIRRTL/Transforms/GrandCentralTaps.cpp b/lib/Dialect/FIRRTL/Transforms/GrandCentralTaps.cpp index c2e5e354ec24..f861beaa820d 100644 --- a/lib/Dialect/FIRRTL/Transforms/GrandCentralTaps.cpp +++ b/lib/Dialect/FIRRTL/Transforms/GrandCentralTaps.cpp @@ -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. diff --git a/test/Conversion/ExportVerilog/sv-interfaces.mlir b/test/Conversion/ExportVerilog/sv-interfaces.mlir index 7ef7355e0120..aaa197ae0258 100644 --- a/test/Conversion/ExportVerilog/sv-interfaces.mlir +++ b/test/Conversion/ExportVerilog/sv-interfaces.mlir @@ -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"} {} } diff --git a/test/Conversion/FIRRTLToHW/lower-to-hw.mlir b/test/Conversion/FIRRTLToHW/lower-to-hw.mlir index 851accbbc1ae..e6522df46dd7 100644 --- a/test/Conversion/FIRRTLToHW/lower-to-hw.mlir +++ b/test/Conversion/FIRRTLToHW/lower-to-hw.mlir @@ -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" + } {} } diff --git a/test/Dialect/FIRRTL/grand-central.mlir b/test/Dialect/FIRRTL/grand-central.mlir index d5469cf52b20..05838e1b778e 100644 --- a/test/Dialect/FIRRTL/grand-central.mlir +++ b/test/Dialect/FIRRTL/grand-central.mlir @@ -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" @@ -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> @@ -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 @@ -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>> // ----- @@ -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 @@ -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 @@ -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 "// string;" // CHECK-NEXT: sv.verbatim "// boolean;" // CHECK-NEXT: sv.verbatim "// integer;" @@ -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 // ----- @@ -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 // ----- @@ -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() @@ -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>> @@ -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 "// bar[2][3];"