From 736142c6e8965d531f1ec47454349c530f2632df Mon Sep 17 00:00:00 2001 From: Prithayan Barua Date: Mon, 12 Aug 2024 10:07:24 -0400 Subject: [PATCH] [CreateSifiveMetadata] Update memory hierarchy paths to be pre-extrction (#7491) The `CreateSifiveMetadata` pass was creating hierarchy paths to the memory module pre-extraction. The `om.path` was being updated as the memory was extracted in the following passes. But recently we realized this was a bug, as the downstream tools consuming the metadata files, were actually expecting the pre-extraction hierarchy paths to the memory. This PR fixes the path, to terminate at the parent of the module that instantiates the memory and encodes the pre-extraction memory instance as a string. The tool that will parse the final `mlir` must now construct the actual pre-extraction path from the two lists, by appending the pre-extraction instance name to the path. --- .../Transforms/CreateSiFiveMetadata.cpp | 122 +++++++++++------- test/Dialect/FIRRTL/SFCTests/directories.fir | 3 +- test/Dialect/FIRRTL/emit-metadata.mlir | 27 +++- test/firtool/prefixMemory.fir | 6 +- 4 files changed, 97 insertions(+), 61 deletions(-) diff --git a/lib/Dialect/FIRRTL/Transforms/CreateSiFiveMetadata.cpp b/lib/Dialect/FIRRTL/Transforms/CreateSiFiveMetadata.cpp index 30aaee22ee90..d84150e9798f 100644 --- a/lib/Dialect/FIRRTL/Transforms/CreateSiFiveMetadata.cpp +++ b/lib/Dialect/FIRRTL/Transforms/CreateSiFiveMetadata.cpp @@ -46,7 +46,7 @@ struct ObjectModelIR { ObjectModelIR( CircuitOp circtOp, FModuleOp dutMod, InstanceGraph &instanceGraph, DenseMap &moduleNamespaces) - : circtOp(circtOp), dutMod(dutMod), + : context(circtOp->getContext()), circtOp(circtOp), dutMod(dutMod), circtNamespace(CircuitNamespace(circtOp)), instancePathCache(InstancePathCache(instanceGraph)), moduleNamespaces(moduleNamespaces) {} @@ -54,20 +54,22 @@ struct ObjectModelIR { // Add the tracker annotation to the op and get a PathOp to the op. PathOp createPathRef(Operation *op, hw::HierPathOp nla, mlir::ImplicitLocOpBuilder &builderOM) { - auto *context = op->getContext(); - NamedAttrList fields; auto id = DistinctAttr::create(UnitAttr::get(context)); - fields.append("id", id); - fields.append("class", StringAttr::get(context, "circt.tracker")); - if (nla) - fields.append("circt.nonlocal", mlir::FlatSymbolRefAttr::get(nla)); - AnnotationSet annos(op); - annos.addAnnotations(DictionaryAttr::get(context, fields)); - annos.applyToOperation(op); TargetKind kind = TargetKind::Reference; - if (isa(op)) - kind = TargetKind::Instance; + // If op is null, then create an empty path. + if (op) { + NamedAttrList fields; + fields.append("id", id); + fields.append("class", StringAttr::get(context, "circt.tracker")); + if (nla) + fields.append("circt.nonlocal", mlir::FlatSymbolRefAttr::get(nla)); + AnnotationSet annos(op); + annos.addAnnotations(DictionaryAttr::get(context, fields)); + annos.applyToOperation(op); + if (isa(op)) + kind = TargetKind::Instance; + } // Create the path operation. return builderOM.create(kind, id); @@ -101,7 +103,6 @@ struct ObjectModelIR { } void createMemorySchema() { - auto *context = circtOp.getContext(); auto unknownLoc = mlir::UnknownLoc::get(context); auto builderOM = mlir::ImplicitLocOpBuilder::atBlockEnd( @@ -121,7 +122,7 @@ struct ObjectModelIR { buildSimpleClassOp(builderOM, unknownLoc, "ExtraPortsMemorySchema", extraPortFields, extraPortsType); - mlir::Type classFieldTypes[12] = { + mlir::Type classFieldTypes[13] = { StringType::get(context), FIntegerType::get(context), FIntegerType::get(context), @@ -133,9 +134,11 @@ struct ObjectModelIR { FIntegerType::get(context), ListType::get(context, cast(PathType::get(context))), BoolType::get(context), - ListType::get(context, - cast(detail::getInstanceTypeForClassLike( - extraPortsClass)))}; + ListType::get( + context, cast( + detail::getInstanceTypeForClassLike(extraPortsClass))), + ListType::get(context, cast(StringType::get(context))), + }; memorySchemaClass = buildSimpleClassOp(builderOM, unknownLoc, "MemorySchema", @@ -149,7 +152,6 @@ struct ObjectModelIR { } void createRetimeModulesSchema() { - auto *context = circtOp.getContext(); auto unknownLoc = mlir::UnknownLoc::get(context); auto builderOM = mlir::ImplicitLocOpBuilder::atBlockEnd( unknownLoc, circtOp.getBodyBlock()); @@ -189,7 +191,6 @@ struct ObjectModelIR { } void addBlackBoxModulesSchema() { - auto *context = circtOp.getContext(); auto unknownLoc = mlir::UnknownLoc::get(context); auto builderOM = mlir::ImplicitLocOpBuilder::atBlockEnd( unknownLoc, circtOp.getBodyBlock()); @@ -232,7 +233,6 @@ struct ObjectModelIR { createMemorySchema(); auto builderOM = mlir::ImplicitLocOpBuilder::atBlockEnd( mem.getLoc(), memoryMetadataClass.getBodyBlock()); - auto *context = builderOM.getContext(); auto createConstField = [&](Attribute constVal) -> Value { if (auto boolConstant = dyn_cast_or_null(constVal)) return builderOM.create(boolConstant); @@ -246,32 +246,52 @@ struct ObjectModelIR { auto memPaths = instancePathCache.getAbsolutePaths(mem); SmallVector memoryHierPaths; - for (auto memPath : memPaths) { - Operation *finalInst = memPath.leaf(); - SmallVector namepath; - bool foundDut = dutMod == nullptr; - for (auto inst : memPath) { - if (!foundDut) - if (inst->getParentOfType() == dutMod) - foundDut = true; - if (!foundDut) - continue; + SmallVector finalInstanceNames; + // Memory hierarchy is relevant only for memories under DUT. + if (inDut) { + for (auto memPath : memPaths) { + { + igraph::InstanceOpInterface finalInst = memPath.leaf(); + finalInstanceNames.emplace_back(builderOM.create( + finalInst.getInstanceNameAttr())); + } + SmallVector namepath; + bool foundDut = dutMod == nullptr; + // The hierpath will be created to the pre-extracted + // instance, thus drop the leaf instance of the path, which can be + // extracted in subsequent passes. + igraph::InstanceOpInterface preExtractedLeafInstance; + for (auto inst : llvm::drop_end(memPath)) { + if (!foundDut) + if (inst->getParentOfType() == dutMod) + foundDut = true; + if (!foundDut) + continue; - namepath.emplace_back(firrtl::getInnerRefTo( - inst, [&](auto mod) -> hw::InnerSymbolNamespace & { - return getModuleNamespace(mod); - })); - } - if (namepath.empty()) - continue; - auto nla = nlaBuilder.create( - mem->getLoc(), - nlaBuilder.getStringAttr(circtNamespace.newName("memNLA")), - nlaBuilder.getArrayAttr(namepath)); + namepath.emplace_back(firrtl::getInnerRefTo( + inst, [&](auto mod) -> hw::InnerSymbolNamespace & { + return getModuleNamespace(mod); + })); + preExtractedLeafInstance = inst; + } + PathOp pathRef; + if (!namepath.empty()) { + auto nla = nlaBuilder.create( + mem->getLoc(), + nlaBuilder.getStringAttr(circtNamespace.newName("memNLA")), + nlaBuilder.getArrayAttr(namepath)); + pathRef = createPathRef(preExtractedLeafInstance, nla, builderOM); + } else { + pathRef = createPathRef({}, {}, builderOM); + } - // Create the path operation. - memoryHierPaths.emplace_back(createPathRef(finalInst, nla, builderOM)); + // Create the path operation. + memoryHierPaths.push_back(pathRef); + } } + auto finalInstNamesList = builderOM.create( + ListType::get(context, cast(StringType::get(context))), + finalInstanceNames); auto hierpaths = builderOM.create( ListType::get(context, cast(PathType::get(context))), memoryHierPaths); @@ -313,10 +333,13 @@ struct ObjectModelIR { .Case("writeLatency", mem.getWriteLatencyAttr()) .Case("hierarchy", {}) .Case("inDut", BoolAttr::get(context, inDut)) - .Case("extraPorts", {})); + .Case("extraPorts", {}) + .Case("preExtInstName", {})); if (!propVal) { if (field.value() == "hierarchy") propVal = hierpaths; + else if (field.value() == "preExtInstName") + propVal = finalInstNamesList; else propVal = extraPorts; } @@ -408,7 +431,6 @@ struct ObjectModelIR { // Create the path ref op and record it. pathOpsToDut.emplace_back(createPathRef(leafInst, nla, builder)); } - auto *context = builder.getContext(); // Create the list of paths op and add it as a field of the class. auto pathList = builder.create( ListType::get(context, cast(PathType::get(context))), @@ -425,6 +447,7 @@ struct ObjectModelIR { hw::InnerSymbolNamespace &getModuleNamespace(FModuleLike module) { return moduleNamespaces.try_emplace(module, module).first->second; } + MLIRContext *context; CircuitOp circtOp; FModuleOp dutMod; CircuitNamespace circtNamespace; @@ -435,10 +458,11 @@ struct ObjectModelIR { ClassOp memoryMetadataClass; ClassOp retimeModulesMetadataClass, retimeModulesSchemaClass; ClassOp blackBoxModulesSchemaClass, blackBoxMetadataClass; - StringRef memoryParamNames[12] = { - "name", "depth", "width", "maskBits", - "readPorts", "writePorts", "readwritePorts", "writeLatency", - "readLatency", "hierarchy", "inDut", "extraPorts"}; + StringRef memoryParamNames[13] = { + "name", "depth", "width", "maskBits", + "readPorts", "writePorts", "readwritePorts", "writeLatency", + "readLatency", "hierarchy", "inDut", "extraPorts", + "preExtInstName"}; StringRef retimeModulesParamNames[1] = {"moduleName"}; StringRef blackBoxModulesParamNames[1] = {"moduleName"}; llvm::SmallDenseSet blackboxModules; diff --git a/test/Dialect/FIRRTL/SFCTests/directories.fir b/test/Dialect/FIRRTL/SFCTests/directories.fir index c1d576c99ec4..1c4c13fcd426 100644 --- a/test/Dialect/FIRRTL/SFCTests/directories.fir +++ b/test/Dialect/FIRRTL/SFCTests/directories.fir @@ -151,7 +151,7 @@ circuit TestHarness: ; MLIR_OUT: om.class.field @[[V3:.+]], %5 : !om.class.type<@SitestBlackBoxModulesSchema> ; MLIR_OUT: } -; MLIR_OUT: om.class @MemorySchema(%basepath: !om.basepath, %name_in: !om.string, %depth_in: !om.integer, %width_in: !om.integer, %maskBits_in: !om.integer, %readPorts_in: !om.integer, %writePorts_in: !om.integer, %readwritePorts_in: !om.integer, %writeLatency_in: !om.integer, %readLatency_in: !om.integer, %hierarchy_in: !om.list, %inDut_in: i1, %extraPorts_in: !om.list>) +; MLIR_OUT: om.class @MemorySchema(%basepath: !om.basepath, %name_in: !om.string, %depth_in: !om.integer, %width_in: !om.integer, %maskBits_in: !om.integer, %readPorts_in: !om.integer, %writePorts_in: !om.integer, %readwritePorts_in: !om.integer, %writeLatency_in: !om.integer, %readLatency_in: !om.integer, %hierarchy_in: !om.list, %inDut_in: i1, %extraPorts_in: !om.list> ; MLIR_OUT: om.class.field @name, %name_in : !om.string ; MLIR_OUT: om.class.field @depth, %depth_in : !om.integer ; MLIR_OUT: om.class.field @width, %width_in : !om.integer @@ -163,7 +163,6 @@ circuit TestHarness: ; MLIR_OUT: om.class.field @readLatency, %readLatency_in : !om.integer ; MLIR_OUT: om.class.field @hierarchy, %hierarchy_in : !om.list ; MLIR_OUT: om.class.field @extraPorts, %extraPorts_in : !om.list> -; MLIR_OUT: } ; MLIR_OUT: om.class @MemoryMetadata(%basepath: !om.basepath) ; MLIR_OUT: om.path_create instance %basepath @memNLA ; MLIR_OUT: om.list_create diff --git a/test/Dialect/FIRRTL/emit-metadata.mlir b/test/Dialect/FIRRTL/emit-metadata.mlir index 5f4dec917531..3759433e4f08 100644 --- a/test/Dialect/FIRRTL/emit-metadata.mlir +++ b/test/Dialect/FIRRTL/emit-metadata.mlir @@ -187,7 +187,7 @@ firrtl.circuit "top" // CHECK-LABEL: firrtl.circuit "OneMemory" firrtl.circuit "OneMemory" { firrtl.module @OneMemory() { - %0:5= firrtl.instance MWrite_ext sym @MWrite_ext_0 @MWrite_ext(in W0_addr: !firrtl.uint<4>, in W0_en: !firrtl.uint<1>, in W0_clk: !firrtl.clock, in W0_data: !firrtl.uint<42>, in user_input: !firrtl.uint<5>) + %0:5= firrtl.instance MWrite_ext_inst sym @MWrite_ext_0 @MWrite_ext(in W0_addr: !firrtl.uint<4>, in W0_en: !firrtl.uint<1>, in W0_clk: !firrtl.clock, in W0_data: !firrtl.uint<42>, in user_input: !firrtl.uint<5>) } firrtl.memmodule @MWrite_ext(in W0_addr: !firrtl.uint<4>, in W0_en: !firrtl.uint<1>, in W0_clk: !firrtl.clock, in W0_data: !firrtl.uint<42>, in user_input: !firrtl.uint<5>) attributes {dataWidth = 42 : ui32, depth = 12 : ui64, extraPorts = [{direction = "input", name = "user_input", width = 5 : ui32}], maskBits = 1 : ui32, numReadPorts = 0 : ui32, numReadWritePorts = 0 : ui32, numWritePorts = 1 : ui32, readLatency = 1 : ui32, writeLatency = 1 : ui32} @@ -203,9 +203,11 @@ firrtl.circuit "OneMemory" { // CHECK: firrtl.propassign %writeLatency, %writeLatency_in : !firrtl.integer // CHECK: firrtl.propassign %readLatency, %readLatency_in : !firrtl.integer // CHECK: firrtl.propassign %hierarchy, %hierarchy_in : !firrtl.list - + // CHECK: firrtl.propassign %preExtInstName, %preExtInstName_in : !firrtl.list // CHECK: firrtl.class @MemoryMetadata - // CHECK: firrtl.path instance distinct[0]<> + // CHECK: %[[V0:.+]] = firrtl.string "MWrite_ext_inst" + // CHECK: %[[V1:.+]] = firrtl.path reference distinct[0]<> + // CHECK: %[[V2:.+]] = firrtl.list.create %[[V0]] : !firrtl.list // CHECK: %MWrite_ext = firrtl.object @MemorySchema // CHECK: firrtl.string "MWrite_ext" // CHECK: firrtl.object.subfield %MWrite_ext[name_in] @@ -226,11 +228,13 @@ firrtl.circuit "OneMemory" { // CHECK: firrtl.integer 1 // CHECK: firrtl.object.subfield %MWrite_ext[readLatency_in] // CHECK: firrtl.object.subfield %MWrite_ext[hierarchy_in] + // CHECK: %[[V33:.+]] = firrtl.object.subfield %MWrite_ext[preExtInstName_in] + // CHECK: firrtl.propassign %[[V33]], %[[V2]] : !firrtl.list // CHECK: firrtl.propassign %MWrite_ext_field, %MWrite_ext // CHECK: } // CHECK: emit.file "metadata{{/|\\\\}}seq_mems.json" { - // CHECK-NEXT{LITERAL}: sv.verbatim "[\0A {\0A \22module_name\22: \22{{0}}\22,\0A \22depth\22: 12,\0A \22width\22: 42,\0A \22masked\22: false,\0A \22read\22: 0,\0A \22write\22: 1,\0A \22readwrite\22: 0,\0A \22extra_ports\22: [\0A {\0A \22name\22: \22user_input\22,\0A \22direction\22: \22input\22,\0A \22width\22: 5\0A }\0A ],\0A \22hierarchy\22: [\0A \22{{1}}.MWrite_ext\22\0A ]\0A }\0A]" + // CHECK-NEXT{LITERAL}: sv.verbatim "[\0A {\0A \22module_name\22: \22{{0}}\22,\0A \22depth\22: 12,\0A \22width\22: 42,\0A \22masked\22: false,\0A \22read\22: 0,\0A \22write\22: 1,\0A \22readwrite\22: 0,\0A \22extra_ports\22: [\0A {\0A \22name\22: \22user_input\22,\0A \22direction\22: \22input\22,\0A \22width\22: 5\0A }\0A ],\0A \22hierarchy\22: [\0A \22{{1}}.MWrite_ext_inst\22\0A ]\0A }\0A]" // CHECK-SAME: {symbols = [@MWrite_ext, @OneMemory]} // CHECK-NEXT: } @@ -272,8 +276,9 @@ firrtl.circuit "ReadOnlyMemory" { // CHECK-LABEL: firrtl.circuit "top" firrtl.circuit "top" { // CHECK: hw.hierpath @[[DUTNLA:.+]] [@top::@sym] + // CHECK-LABEL: firrtl.module @top firrtl.module @top() { - // CHECK: firrtl.instance dut sym @[[DUT_SYM:.+]] {annotations = [{circt.nonlocal = @dutNLA, class = "circt.tracker", id = distinct[0]<>}]} @DUT() + // CHECK: firrtl.instance dut sym @[[DUT_SYM:.+]] {annotations = [{circt.nonlocal = @dutNLA, class = "circt.tracker", id = distinct[0]<>}]} @DUT() firrtl.instance dut @DUT() firrtl.instance mem1 @Mem1() firrtl.instance mem2 @Mem2() @@ -284,9 +289,11 @@ firrtl.circuit "top" { firrtl.module private @Mem2() { %0:4 = firrtl.instance head_0_ext @head_0_ext(in W0_addr: !firrtl.uint<5>, in W0_en: !firrtl.uint<1>, in W0_clk: !firrtl.clock, in W0_data: !firrtl.uint<5>) } + // CHECK-LABEL: firrtl.module private @DUT( firrtl.module private @DUT() attributes {annotations = [ {class = "sifive.enterprise.firrtl.MarkDUTAnnotation"}]} { - // CHECK: firrtl.instance mem1 sym @[[MEM1_SYM:.+]] @Mem( + // CHECK: firrtl.instance mem1 sym @[[MEM1_SYM:.+]] {annotations = [{ + // CHECK-SAME: circt.nonlocal = @memNLA, class = "circt.tracker", id = distinct firrtl.instance mem1 @Mem() } firrtl.module private @Mem() { @@ -298,6 +305,12 @@ firrtl.circuit "top" { firrtl.memmodule private @memory_ext(in R0_addr: !firrtl.uint<4>, in R0_en: !firrtl.uint<1>, in R0_clk: !firrtl.clock, out R0_data: !firrtl.uint<8>, in RW0_addr: !firrtl.uint<4>, in RW0_en: !firrtl.uint<1>, in RW0_clk: !firrtl.clock, in RW0_wmode: !firrtl.uint<1>, in RW0_wdata: !firrtl.uint<8>, out RW0_rdata: !firrtl.uint<8>) attributes {dataWidth = 8 : ui32, depth = 16 : ui64, extraPorts = [], maskBits = 1 : ui32, numReadPorts = 1 : ui32, numReadWritePorts = 1 : ui32, numWritePorts = 0 : ui32, readLatency = 1 : ui32, writeLatency = 1 : ui32} firrtl.memmodule private @dumm_ext(in R0_addr: !firrtl.uint<5>, in R0_en: !firrtl.uint<1>, in R0_clk: !firrtl.clock, out R0_data: !firrtl.uint<5>, in W0_addr: !firrtl.uint<5>, in W0_en: !firrtl.uint<1>, in W0_clk: !firrtl.clock, in W0_data: !firrtl.uint<5>) attributes {dataWidth = 5 : ui32, depth = 20 : ui64, extraPorts = [], maskBits = 1 : ui32, numReadPorts = 1 : ui32, numReadWritePorts = 0 : ui32, numWritePorts = 1 : ui32, readLatency = 1 : ui32, writeLatency = 1 : ui32} + // CHECK-LABEL: firrtl.class @MemoryMetadata + // CHECK: %[[V2:.+]] = firrtl.string "memory_ext" + // CHECK: %[[V3:.+]] = firrtl.path instance distinct[1]<> + // CHECK: firrtl.list.create %[[V2]] : !firrtl.list + // CHECK: firrtl.list.create %[[V3]] : !firrtl.list + // CHECK: emit.file "metadata{{/|\\\\}}seq_mems.json" { // CHECK-NEXT{LITERAL}: sv.verbatim "[\0A {\0A \22module_name\22: \22{{0}}\22,\0A \22depth\22: 16,\0A \22width\22: 8,\0A \22masked\22: false,\0A \22read\22: 1,\0A \22write\22: 0,\0A \22readwrite\22: 1,\0A \22extra_ports\22: [],\0A \22hierarchy\22: [\0A \22{{3}}.{{4}}.memory_ext\22\0A ]\0A },\0A {\0A \22module_name\22: \22{{5}}\22,\0A \22depth\22: 20,\0A \22width\22: 5,\0A \22masked\22: false,\0A \22read\22: 1,\0A \22write\22: 1,\0A \22readwrite\22: 0,\0A \22extra_ports\22: [],\0A \22hierarchy\22: [\0A \22{{3}}.{{4}}.dumm_ext\22\0A ]\0A }\0A]" // CHECK-SAME: {symbols = [@memory_ext, @top, #hw.innerNameRef<@top::@[[DUT_SYM]]>, @DUT, #hw.innerNameRef<@DUT::@[[MEM1_SYM]]>, @dumm_ext]} @@ -308,7 +321,7 @@ firrtl.circuit "top" { // CHECK-SAME: {symbols = [@head_ext, @head_0_ext, @memory_ext, @dumm_ext]} // CHECK-NEXT: } - // CHECK: firrtl.class @SiFive_Metadata + // CHECK-LABEL: firrtl.class @SiFive_Metadata // CHECK: %[[V0:.+]] = firrtl.path instance distinct[0]<> // CHECK-NEXT: %[[V1:.+]] = firrtl.list.create %[[V0]] : !firrtl.list // CHECK-NEXT: firrtl.propassign %dutModulePath_field_1, %[[V1]] : !firrtl.list diff --git a/test/firtool/prefixMemory.fir b/test/firtool/prefixMemory.fir index 253b0c31a964..974cfee939ef 100644 --- a/test/firtool/prefixMemory.fir +++ b/test/firtool/prefixMemory.fir @@ -37,7 +37,7 @@ circuit Foo : %[[ input writeAddr : UInt<3> input writeData : UInt<32> - ; REPL-FIR: firrtl.instance mem sym @{{[^ ]+}} @prefix1_mem + ; REPL-FIR: firrtl.instance mem sym @{{[^ ]+}} {annotations = [{circt.nonlocal = @memNLA, class = "circt.tracker", id = distinct[0]<>}]} @prefix1_mem ; REPL-HW: hw.instance "mem" sym @{{[^ ]+}} @prefix1_mem ; SIM-FIR: firrtl.mem ; SIM-FIR-SAME: name = "mem" @@ -69,7 +69,6 @@ circuit Foo : %[[ mem.MPORT.data <= writeData mem.MPORT.mask <= UInt<1>(0h1) - ; CHECK-FIR-LABEL: firrtl.module private @prefix2_Baz ; CHECK-HW-LABEL: hw.module private @prefix2_Baz module Baz : input clock : Clock @@ -80,7 +79,8 @@ circuit Foo : %[[ input writeAddr : UInt<3> input writeData : UInt<32> - ; REPL-FIR: firrtl.instance mem sym @{{[^ ]+}} @prefix2_mem + ; REPL-FIR: firrtl.module private @prefix2_Baz + ; REPL-FIR-NEXT: firrtl.instance mem sym @sym {annotations = [{circt.nonlocal = @memNLA_0, class = "circt.tracker", id = distinct[1]<>}]} @prefix2_mem_0 ; REPL-HW: hw.instance "mem" sym @{{[^ ]+}} @prefix2_mem ; SIM-FIR: firrtl.mem ; SIM-FIR-SAME: name = "mem"