From 49c6f83c7f22c6460f0fba7ccb018ba009433651 Mon Sep 17 00:00:00 2001 From: Schuyler Eldridge Date: Fri, 13 Sep 2024 22:41:10 -0400 Subject: [PATCH] [FIRRTL] Add layer block support to AddSeqMemPorts Update the AddSeqMemPorts pass to support layers. Namely, this will now allow for input ports to be added to memories declared under layer block. If a user tries to add output ports, then produce an error. Generally, this entire pass likely doesn't make sense for layer blocks at all. I.e., this pass is entirely about adding MBIST ports to memories. Memories that are declared under layerblocks are likely only for verification use and adding MBIST ports to these doesn't really make sense. Nonetheless, it is reasonable to have this pass do something sane if it encounters a user request like this. This made a small change to the pass output where the connects created are placed immediately following the memory instance. This is necessary to create the connects inside the layer blocks. However, it is also slightly better output than before. (This changed one test superficially.) Signed-off-by: Schuyler Eldridge --- .../FIRRTL/Transforms/AddSeqMemPorts.cpp | 38 ++++++++++++-- .../FIRRTL/add-seqmem-ports-errors.mlir | 47 +++++++++++++++++ test/Dialect/FIRRTL/add-seqmem-ports.mlir | 50 ++++++++++++++++++- 3 files changed, 129 insertions(+), 6 deletions(-) diff --git a/lib/Dialect/FIRRTL/Transforms/AddSeqMemPorts.cpp b/lib/Dialect/FIRRTL/Transforms/AddSeqMemPorts.cpp index 53413881f14f..4fab12d21311 100644 --- a/lib/Dialect/FIRRTL/Transforms/AddSeqMemPorts.cpp +++ b/lib/Dialect/FIRRTL/Transforms/AddSeqMemPorts.cpp @@ -196,8 +196,7 @@ void AddSeqMemPortsPass::processMemModule(FMemModuleOp mem) { LogicalResult AddSeqMemPortsPass::processModule(FModuleOp module, bool isDUT) { auto *context = &getContext(); - // Insert the new port connections at the end of the module. - auto builder = OpBuilder::atBlockEnd(module.getBodyBlock()); + auto builder = OpBuilder(module.getContext()); auto &memInfo = memInfoMap[module]; auto &extraPorts = memInfo.extraPorts; // List of ports added to submodules which must be connected to this module's @@ -207,7 +206,7 @@ LogicalResult AddSeqMemPortsPass::processModule(FModuleOp module, bool isDUT) { // The base index to use when adding ports to the current module. unsigned firstPortIndex = module.getNumPorts(); - for (auto &op : llvm::make_early_inc_range(*module.getBodyBlock())) { + auto result = module.walk([&](Operation *op) { if (auto inst = dyn_cast(op)) { auto submodule = inst.getReferencedModule(*instanceGraph); @@ -215,7 +214,7 @@ LogicalResult AddSeqMemPortsPass::processModule(FModuleOp module, bool isDUT) { // If there are no extra ports, we don't have to do anything. if (subMemInfoIt == memInfoMap.end() || subMemInfoIt->second.extraPorts.empty()) - continue; + return WalkResult::advance(); auto &subMemInfo = subMemInfoIt->second; // Find out how many memory ports we have to add. auto &subExtraPorts = subMemInfo.extraPorts; @@ -240,6 +239,20 @@ LogicalResult AddSeqMemPortsPass::processModule(FModuleOp module, bool isDUT) { StringAttr::get(context, "sram_" + Twine(sramIndex) + "_" + sramPort.name.getValue()); auto portDirection = sramPort.direction; + // If the port direction is output, then ensure that this memory is not + // under a layerblock. + if (portDirection == Direction::Out) { + auto layerParent = inst->getParentOfType(); + if (layerParent) { + auto diag = submodule->emitOpError() + << "cannot have an output port added to it because it " + "is instantiated under a layer block"; + diag.attachNote(layerParent.getLoc()) + << "the memory is instantiated under this layer block"; + diag.attachNote(inst.getLoc()) << "the memory is instantiated here"; + return WalkResult::interrupt(); + } + } auto portType = sramPort.type; // Record the extra port. extraPorts.push_back( @@ -272,20 +285,35 @@ LogicalResult AddSeqMemPortsPass::processModule(FModuleOp module, bool isDUT) { instancePaths.back().push_back(ref); } } + + return WalkResult::advance(); } - } + + return WalkResult::advance(); + }); + + if (result.wasInterrupted()) + return failure(); // Add the extra ports to this module. module.insertPorts(extraPorts); // Connect the submodule ports to the parent module ports. + DenseMap instToInsertionPoint; for (unsigned i = 0, e = values.size(); i < e; ++i) { auto &[firstArg, port] = extraPorts[i]; Value modulePort = module.getArgument(firstArg + i); Value instPort = values[i]; + Operation *instOp = instPort.getDefiningOp(); + auto insertPoint = instToInsertionPoint.find(instOp); + if (insertPoint == instToInsertionPoint.end()) + builder.setInsertionPointAfter(instOp); + else + builder.restoreInsertionPoint(insertPoint->getSecond()); if (port.direction == Direction::In) std::swap(modulePort, instPort); builder.create(port.loc, modulePort, instPort); + instToInsertionPoint[instOp] = builder.saveInsertionPoint(); } return success(); } diff --git a/test/Dialect/FIRRTL/add-seqmem-ports-errors.mlir b/test/Dialect/FIRRTL/add-seqmem-ports-errors.mlir index 35e25f679662..821b840a4b62 100644 --- a/test/Dialect/FIRRTL/add-seqmem-ports-errors.mlir +++ b/test/Dialect/FIRRTL/add-seqmem-ports-errors.mlir @@ -63,3 +63,50 @@ firrtl.circuit "Simple" attributes {annotations = [{ }]} { firrtl.module @Simple() { } } + +// ----- + +firrtl.circuit "Foo" attributes { + annotations = [ + { + class = "sifive.enterprise.firrtl.AddSeqMemPortAnnotation", + name = "user_input", + input = false, + width = 1 + } + ] +} { + firrtl.layer @A bind {} + + // expected-error @below {{cannot have an output port added to it because it is instantiated under a layer block}} + firrtl.memmodule @mem_ext( + in W0_addr: !firrtl.uint<1>, + in W0_en: !firrtl.uint<1>, + in W0_clk: !firrtl.clock, + in W0_data: !firrtl.uint<1> + ) attributes { + dataWidth = 1 : ui32, + depth = 2 : ui64, + extraPorts = [], + maskBits = 1 : ui32, + numReadPorts = 0 : ui32, + numReadWritePorts = 0 : ui32, + numWritePorts = 1 : ui32, + readLatency = 1 : ui32, + writeLatency = 1 : ui32 + } + + firrtl.module @Foo() { + // expected-note @below {{the memory is instantiated under this layer block}} + firrtl.layerblock @A { + // expected-note @below {{the memory is instantiated here}} + %0:4 = firrtl.instance MWrite_ext @mem_ext( + in W0_addr: !firrtl.uint<1>, + in W0_en: !firrtl.uint<1>, + in W0_clk: !firrtl.clock, + in W0_data: !firrtl.uint<1> + ) + } + } + +} diff --git a/test/Dialect/FIRRTL/add-seqmem-ports.mlir b/test/Dialect/FIRRTL/add-seqmem-ports.mlir index 6d1e1f1ea53e..2078b9e78485 100644 --- a/test/Dialect/FIRRTL/add-seqmem-ports.mlir +++ b/test/Dialect/FIRRTL/add-seqmem-ports.mlir @@ -100,9 +100,9 @@ firrtl.circuit "Two" attributes {annotations = [ firrtl.instance child0 @Child() firrtl.instance child1 @Child() // CHECK: %child0_sram_0_user_output, %child0_sram_0_user_input = firrtl.instance child0 @Child - // CHECK: %child1_sram_0_user_output, %child1_sram_0_user_input = firrtl.instance child1 @Child // CHECK: firrtl.matchingconnect %sram_0_user_output, %child0_sram_0_user_output : !firrtl.uint<4> // CHECK: firrtl.matchingconnect %child0_sram_0_user_input, %sram_0_user_input : !firrtl.uint<3> + // CHECK: %child1_sram_0_user_output, %child1_sram_0_user_input = firrtl.instance child1 @Child // CHECK: firrtl.matchingconnect %sram_1_user_output, %child1_sram_0_user_output : !firrtl.uint<4> // CHECK: firrtl.matchingconnect %child1_sram_0_user_input, %sram_1_user_input : !firrtl.uint<3> @@ -191,3 +191,51 @@ firrtl.circuit "Complex" attributes {annotations = [ // CHECK-SAME: {symbols = [@DUT, @[[memNLA]], @[[memNLA_0]], @[[memNLA_1]]]} // CHECK-NEXT: } } + +// CHECK-LABEL: firrtl.circuit "LayerBlock" +firrtl.circuit "LayerBlock" attributes { + annotations = [ + { + class = "sifive.enterprise.firrtl.AddSeqMemPortAnnotation", + name = "user_input", + input = true, + width = 1 + } + ] +} { + firrtl.layer @A bind {} + + // CHECK: firrtl.memmodule + // CHECK-SAME: extraPorts = [{direction = "input", name = "user_input", width = 1 : ui32}] + firrtl.memmodule @mem_ext( + in W0_addr: !firrtl.uint<1>, + in W0_en: !firrtl.uint<1>, + in W0_clk: !firrtl.clock, + in W0_data: !firrtl.uint<1> + ) attributes { + dataWidth = 1 : ui32, + depth = 2 : ui64, + extraPorts = [], + maskBits = 1 : ui32, + numReadPorts = 0 : ui32, + numReadWritePorts = 0 : ui32, + numWritePorts = 1 : ui32, + readLatency = 1 : ui32, + writeLatency = 1 : ui32 + } + + firrtl.module @LayerBlock() { + // CHECK: firrtl.layerblock @A + firrtl.layerblock @A { + // CHECK-NEXT: firrtl.instance + %0:4 = firrtl.instance MWrite_ext @mem_ext( + in W0_addr: !firrtl.uint<1>, + in W0_en: !firrtl.uint<1>, + in W0_clk: !firrtl.clock, + in W0_data: !firrtl.uint<1> + ) + // CHECK-NEXT: firrtl.matchingconnect + } + } + +}