From cdf32d1bc4bb570d6847bbeccd1f2c384073308d Mon Sep 17 00:00:00 2001 From: Prithayan Barua Date: Tue, 26 Apr 2022 22:18:23 -0700 Subject: [PATCH] [FIRRTL] Make GroupID memory attribute unsigned. NFC (#2975) Make the GroupID memory attribute as unsigned int, instead of signed. It is being used to distinguish memories that should not be Deduped. There are two passes currently updating the GroupID: PrefixModules pass assigns unique id for each prefix. CreateSifiveMetadata pass assigns unique id for each memory under the testharness hierarchy. This change ensures consistent assumptions and a convention. --- include/circt/Dialect/FIRRTL/FIRRTLDeclarations.td | 2 +- include/circt/Dialect/FIRRTL/FIRRTLOps.h | 4 ++-- lib/Dialect/FIRRTL/FIRRTLOps.cpp | 4 ++-- lib/Dialect/FIRRTL/Transforms/CreateSiFiveMetadata.cpp | 6 +++--- lib/Dialect/FIRRTL/Transforms/PrefixModules.cpp | 8 ++++---- test/Dialect/FIRRTL/inferRW.mlir | 4 ++-- test/Dialect/FIRRTL/lower-types.mlir | 4 ++-- test/firtool/prefixMemory.fir | 4 ++-- 8 files changed, 18 insertions(+), 18 deletions(-) diff --git a/include/circt/Dialect/FIRRTL/FIRRTLDeclarations.td b/include/circt/Dialect/FIRRTL/FIRRTLDeclarations.td index 4ec44aa427..82b8b4629a 100644 --- a/include/circt/Dialect/FIRRTL/FIRRTLDeclarations.td +++ b/include/circt/Dialect/FIRRTL/FIRRTLDeclarations.td @@ -103,7 +103,7 @@ def MemOp : FIRRTLOp<"mem", [HasCustomSSAName]> { AnnotationArrayAttr:$annotations, PortAnnotationsAttr:$portAnnotations, OptionalAttr:$inner_sym, - OptionalAttr:$groupID); + OptionalAttr:$groupID); let results = (outs Variadic:$results); let assemblyFormat = [{ diff --git a/include/circt/Dialect/FIRRTL/FIRRTLOps.h b/include/circt/Dialect/FIRRTL/FIRRTLOps.h index d1b0843639..b03c23ea63 100644 --- a/include/circt/Dialect/FIRRTL/FIRRTLOps.h +++ b/include/circt/Dialect/FIRRTL/FIRRTLOps.h @@ -157,13 +157,13 @@ struct FirMemory { SmallVector writeClockIDs; StringAttr modName; bool isMasked; - size_t groupID; + uint32_t groupID; // Location is carried along but not considered part of the identity of this. Location loc; std::tuple, size_t> + size_t, hw::WUW, SmallVector, uint32_t> getTuple() const { return std::tie(numReadPorts, numWritePorts, numReadWritePorts, dataWidth, depth, readLatency, writeLatency, maskBits, readUnderWrite, diff --git a/lib/Dialect/FIRRTL/FIRRTLOps.cpp b/lib/Dialect/FIRRTL/FIRRTLOps.cpp index a71a05e295..bd2353935b 100644 --- a/lib/Dialect/FIRRTL/FIRRTLOps.cpp +++ b/lib/Dialect/FIRRTL/FIRRTLOps.cpp @@ -1843,9 +1843,9 @@ FirMemory MemOp::getSummary() { op.emitError("'firrtl.mem' should have simple type and known width"); width = 0; } - size_t groupID = 0; + uint32_t groupID = 0; if (auto gID = op.groupIDAttr()) - groupID = gID.getInt(); + groupID = gID.getUInt(); StringAttr modName; if (op->hasAttr("modName")) modName = op->getAttrOfType("modName"); diff --git a/lib/Dialect/FIRRTL/Transforms/CreateSiFiveMetadata.cpp b/lib/Dialect/FIRRTL/Transforms/CreateSiFiveMetadata.cpp index b7dcf967fc..c7eab277c3 100644 --- a/lib/Dialect/FIRRTL/Transforms/CreateSiFiveMetadata.cpp +++ b/lib/Dialect/FIRRTL/Transforms/CreateSiFiveMetadata.cpp @@ -65,13 +65,13 @@ void CreateSiFiveMetadataPass::renameMemory(CircuitOp circuitOp) { // This is a random number to start the groupIDs at, large enough to not // conflict with existing IDs. // TODO: Move this logic out of this pass. - unsigned baseGroupID = std::numeric_limits::max(); + uint32_t baseGroupID = std::numeric_limits::max(); for (auto mod : circuitOp.getOps()) { bool isTestHarness = !dutModuleSet.contains(mod); for (auto memOp : mod.getBody()->getOps()) { if (isTestHarness) - memOp.groupIDAttr( - IntegerAttr::get(IntegerType::get(ctxt, 32), --baseGroupID)); + memOp.groupIDAttr(IntegerAttr::get( + IntegerType::get(ctxt, 32, IntegerType::Unsigned), --baseGroupID)); memOpList.push_back(memOp); auto firMem = memOp.getSummary(); diff --git a/lib/Dialect/FIRRTL/Transforms/PrefixModules.cpp b/lib/Dialect/FIRRTL/Transforms/PrefixModules.cpp index 8c63b23a7c..09a8c5eb5b 100644 --- a/lib/Dialect/FIRRTL/Transforms/PrefixModules.cpp +++ b/lib/Dialect/FIRRTL/Transforms/PrefixModules.cpp @@ -105,7 +105,7 @@ class PrefixModulesPass : public PrefixModulesBase { /// This is a map from a module name to new prefixes to be applied. PrefixMap prefixMap; - DenseMap prefixIdMap; + DenseMap prefixIdMap; /// A map of Grand Central interface ID to prefix. DenseMap interfacePrefixMap; @@ -125,7 +125,7 @@ class PrefixModulesPass : public PrefixModulesBase { /// any referenced module in the prefix map. void PrefixModulesPass::renameModuleBody(std::string prefix, FModuleOp module) { auto *context = module.getContext(); - size_t groupID = 0; + uint32_t groupID = 0; auto iter = prefixIdMap.find(prefix); if (iter == prefixIdMap.end()) { groupID = prefixIdMap.size() + 1; @@ -176,8 +176,8 @@ void PrefixModulesPass::renameModuleBody(std::string prefix, FModuleOp module) { if (auto memOp = dyn_cast(op)) { // Memories will be turned into modules and should be prefixed. memOp.nameAttr(StringAttr::get(context, prefix + memOp.name())); - memOp.groupIDAttr( - IntegerAttr::get(IntegerType::get(context, 32), groupID)); + memOp.groupIDAttr(IntegerAttr::get( + IntegerType::get(context, 32, IntegerType::Unsigned), groupID)); } else if (auto instanceOp = dyn_cast(op)) { auto target = dyn_cast( *instanceGraph->getReferencedModule(instanceOp)); diff --git a/test/Dialect/FIRRTL/inferRW.mlir b/test/Dialect/FIRRTL/inferRW.mlir index b54eb2d0ea..3016aa43ba 100644 --- a/test/Dialect/FIRRTL/inferRW.mlir +++ b/test/Dialect/FIRRTL/inferRW.mlir @@ -6,7 +6,7 @@ firrtl.circuit "TLRAM" { firrtl.module @TLRAM(in %clock: !firrtl.clock, in %reset: !firrtl.asyncreset, in %index: !firrtl.uint<4>, in %index2: !firrtl.uint<4>, in %data_0: !firrtl.uint<8>, in %wen: !firrtl.uint<1>, in %_T_29: !firrtl.uint<1>, out %auto_0: !firrtl.uint<8>) { %mem_MPORT_en = firrtl.wire : !firrtl.uint<1> %mem_MPORT_data_0 = firrtl.wire : !firrtl.uint<8> - %mem_0_MPORT, %mem_0_MPORT_1 = firrtl.mem Undefined {depth = 16 : i64, groupID = 2 : i32, name = "mem_0", portNames = ["MPORT", "MPORT_1"], readLatency = 1 : i32, writeLatency = 1 : i32} : !firrtl.bundle, en: uint<1>, clk: clock, data flip: uint<8>>, !firrtl.bundle, en: uint<1>, clk: clock, data: uint<8>, mask: uint<1>> + %mem_0_MPORT, %mem_0_MPORT_1 = firrtl.mem Undefined {depth = 16 : i64, groupID = 2 : ui32, name = "mem_0", portNames = ["MPORT", "MPORT_1"], readLatency = 1 : i32, writeLatency = 1 : i32} : !firrtl.bundle, en: uint<1>, clk: clock, data flip: uint<8>>, !firrtl.bundle, en: uint<1>, clk: clock, data: uint<8>, mask: uint<1>> %0 = firrtl.subfield %mem_0_MPORT(0) : (!firrtl.bundle, en: uint<1>, clk: clock, data flip: uint<8>>) -> !firrtl.uint<4> firrtl.connect %0, %index2 : !firrtl.uint<4>, !firrtl.uint<4> %1 = firrtl.subfield %mem_0_MPORT(1) : (!firrtl.bundle, en: uint<1>, clk: clock, data flip: uint<8>>) -> !firrtl.uint<1> @@ -35,7 +35,7 @@ firrtl.circuit "TLRAM" { %11 = firrtl.mux(%REG, %mem_MPORT_data_0, %r_0) : (!firrtl.uint<1>, !firrtl.uint<8>, !firrtl.uint<8>) -> !firrtl.uint<8> firrtl.connect %auto_0, %11 : !firrtl.uint<8>, !firrtl.uint<8> -// CHECK: %mem_0_rw = firrtl.mem Undefined {depth = 16 : i64, groupID = 2 : i32, name = "mem_0", portNames = ["rw"], readLatency = 1 : i32, writeLatency = 1 : i32} : !firrtl.bundle, en: uint<1>, clk: clock, rdata flip: uint<8>, wmode: uint<1>, wdata: uint<8>, wmask: uint<1>> +// CHECK: %mem_0_rw = firrtl.mem Undefined {depth = 16 : i64, groupID = 2 : ui32, name = "mem_0", portNames = ["rw"], readLatency = 1 : i32, writeLatency = 1 : i32} : !firrtl.bundle, en: uint<1>, clk: clock, rdata flip: uint<8>, wmode: uint<1>, wdata: uint<8>, wmask: uint<1>> // CHECK: %[[v7:.+]] = firrtl.mux(%[[writeEnable:.+]], %[[writeAddr:.+]], %[[readAddr:.+]]) : (!firrtl.uint<1>, !firrtl.uint<4>, !firrtl.uint<4>) -> !firrtl.uint<4> // CHECK: firrtl.strictconnect %[[v0:.+]], %[[v7]] : !firrtl.uint<4> // CHECK: %[[v8:.+]] = firrtl.or %[[readEnable:.+]], %[[writeEnable]] : (!firrtl.uint<1>, !firrtl.uint<1>) -> !firrtl.uint<1> diff --git a/test/Dialect/FIRRTL/lower-types.mlir b/test/Dialect/FIRRTL/lower-types.mlir index b9a87dbf23..08d0eb3eb5 100644 --- a/test/Dialect/FIRRTL/lower-types.mlir +++ b/test/Dialect/FIRRTL/lower-types.mlir @@ -1488,14 +1488,14 @@ firrtl.module private @Issue2315(in %x: !firrtl.vector, 5>, in %source: // Ensure 0 bit fields are handled properly. firrtl.module @ZeroWideMem(in %clock: !firrtl.clock, in %reset: !firrtl.uint<1>) { - %ram_MPORT = firrtl.mem Undefined {depth = 4 : i64, groupID = 1 : i32, name = "ram", portNames = ["MPORT"], readLatency = 0 : i32, writeLatency = 1 : i32} : !firrtl.bundle, en: uint<1>, clk: clock, data: bundle, b: uint<20>, c: uint<42>>>, mask: bundle, b: uint<1>, c: uint<1>>>> + %ram_MPORT = firrtl.mem Undefined {depth = 4 : i64, groupID = 1 : ui32, name = "ram", portNames = ["MPORT"], readLatency = 0 : i32, writeLatency = 1 : i32} : !firrtl.bundle, en: uint<1>, clk: clock, data: bundle, b: uint<20>, c: uint<42>>>, mask: bundle, b: uint<1>, c: uint<1>>>> //FLATTEN %ram_MPORT = firrtl.mem Undefined {depth = 4 : i64, name = "ram", portNames = ["MPORT"], readLatency = 0 : i32, writeLatency = 1 : i32} : !firrtl.bundle, en: uint<1>, clk: clock, data: uint<62>, mask: uint<31>> } firrtl.module @ZeroBitMasks(in %clock: !firrtl.clock, in %reset: !firrtl.uint<1>, in %io: !firrtl.bundle, b: uint<20>>) { %invalid = firrtl.invalidvalue : !firrtl.bundle, b: uint<1>> %invalid_0 = firrtl.invalidvalue : !firrtl.bundle, b: uint<20>> - %ram_MPORT = firrtl.mem Undefined {depth = 1 : i64, groupID = 1 : i32, name = "ram", portNames = ["MPORT"], readLatency = 0 : i32, writeLatency = 1 : i32} : !firrtl.bundle, en: uint<1>, clk: clock, data: bundle, b: uint<20>>, mask: bundle, b: uint<1>>> + %ram_MPORT = firrtl.mem Undefined {depth = 1 : i64, groupID = 1 : ui32, name = "ram", portNames = ["MPORT"], readLatency = 0 : i32, writeLatency = 1 : i32} : !firrtl.bundle, en: uint<1>, clk: clock, data: bundle, b: uint<20>>, mask: bundle, b: uint<1>>> %3 = firrtl.subfield %ram_MPORT(3) : (!firrtl.bundle, en: uint<1>, clk: clock, data: bundle, b: uint<20>>, mask: bundle, b: uint<1>>>) -> !firrtl.bundle, b: uint<20>> firrtl.strictconnect %3, %invalid_0 : !firrtl.bundle, b: uint<20>> %4 = firrtl.subfield %ram_MPORT(4) : (!firrtl.bundle, en: uint<1>, clk: clock, data: bundle, b: uint<20>>, mask: bundle, b: uint<1>>>) -> !firrtl.bundle, b: uint<1>> diff --git a/test/firtool/prefixMemory.fir b/test/firtool/prefixMemory.fir index 114f71759d..dbe481f72c 100644 --- a/test/firtool/prefixMemory.fir +++ b/test/firtool/prefixMemory.fir @@ -114,9 +114,9 @@ circuit Foo : %[[ readData <= _readData_T ; CHECK-LABEL: firrtl.module private @prefix1_Bar -; CHECK: = firrtl.mem Undefined {depth = 8 : i64, groupID = -2 : i32, modName = "prefix1_mem_ext", name = "prefix1_mem", portNames = ["MPORT", "readData_MPORT"], readLatency = 1 : i32, writeLatency = 1 : i32} +; CHECK: = firrtl.mem Undefined {depth = 8 : i64, groupID = 4294967294 : ui32, modName = "prefix1_mem_ext", name = "prefix1_mem", portNames = ["MPORT", "readData_MPORT"], readLatency = 1 : i32, writeLatency = 1 : i32} ; CHECK-LABEL: firrtl.module private @prefix2_Baz -; CHECK: = firrtl.mem Undefined {depth = 8 : i64, groupID = -3 : i32, modName = "prefix2_mem_ext", name = "prefix2_mem", portNames = ["MPORT", "readData_MPORT"], readLatency = 1 : i32, writeLatency = 1 : i32} +; CHECK: = firrtl.mem Undefined {depth = 8 : i64, groupID = 4294967293 : ui32, modName = "prefix2_mem_ext", name = "prefix2_mem", portNames = ["MPORT", "readData_MPORT"], readLatency = 1 : i32, writeLatency = 1 : i32} ; HW-LABEL: hw.module @prefix2_mem_ext(%R0_addr: i3, %R0_en: i1, %R0_clk: i1, %W0_addr: i3, %W0_en: i1, %W0_clk: i1, %W0_data: i1) -> (R0_data: i1) ; HW-LABEL: hw.module @prefix1_mem_ext(%R0_addr: i3, %R0_en: i1, %R0_clk: i1, %W0_addr: i3, %W0_en: i1, %W0_clk: i1, %W0_data: i1) -> (R0_data: i1)