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 + } + } + +}