Skip to content

Commit

Permalink
[FIRRTL][ModuleInliner] Add a prefix to memory instances (#7279)
Browse files Browse the repository at this point in the history
This PR basically reverts 4cf7bc9 to add a prefix to MemOp instances. Previously we changed to remove prefix since it broke prefixes created in PrefixModules. However #4952 added `prefix` attribute to MemOp so we don't need the workaround anymore. This makes output verilog a lot more LEC friendly. I checked the PR doesn't break prefixes added by PrefixModules on internal designs.
  • Loading branch information
uenoku authored Jul 8, 2024
1 parent 292375d commit 917cde6
Show file tree
Hide file tree
Showing 2 changed files with 8 additions and 12 deletions.
12 changes: 4 additions & 8 deletions lib/Dialect/FIRRTL/Transforms/ModuleInliner.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -682,14 +682,10 @@ bool Inliner::rename(StringRef prefix, Operation *op, InliningLevel &il) {
if (auto scopeOp = dyn_cast<debug::ScopeOp>(op))
return updateDebugScope(scopeOp), false;

// Add a prefix to things that has a "name" attribute. We don't prefix
// memories since it will affect the name of the generated module.
// TODO: We should find a way to prefix the instance of a memory module.
if (!isa<MemOp, SeqMemOp, CombMemOp, MemoryPortOp>(op)) {
if (auto nameAttr = op->getAttrOfType<StringAttr>("name"))
op->setAttr("name", StringAttr::get(op->getContext(),
(prefix + nameAttr.getValue())));
}
// Add a prefix to things that has a "name" attribute.
if (auto nameAttr = op->getAttrOfType<StringAttr>("name"))
op->setAttr("name", StringAttr::get(op->getContext(),
(prefix + nameAttr.getValue())));

// If the operation has an inner symbol, ensure that it is unique. Record
// renames for any NLAs that this participates in if the symbol was renamed.
Expand Down
8 changes: 4 additions & 4 deletions test/Dialect/FIRRTL/inliner.mlir
Original file line number Diff line number Diff line change
Expand Up @@ -256,11 +256,11 @@ firrtl.module @renaming() {
}
firrtl.module @declarations(in %clock : !firrtl.clock, in %u8 : !firrtl.uint<8>, in %reset : !firrtl.asyncreset) attributes {annotations = [{class = "firrtl.passes.InlineAnnotation"}]} {
%c0_ui8 = firrtl.constant 0 : !firrtl.uint<8>
// CHECK: %cmem = chirrtl.combmem : !chirrtl.cmemory<uint<8>, 8>
// CHECK: %myinst_cmem = chirrtl.combmem : !chirrtl.cmemory<uint<8>, 8>
%cmem = chirrtl.combmem : !chirrtl.cmemory<uint<8>, 8>
// CHECK: %mem_read = firrtl.mem Undefined {depth = 1 : i64, name = "mem", portNames = ["read"], readLatency = 0 : i32, writeLatency = 1 : i32} : !firrtl.bundle<addr: uint<1>, en: uint<1>, clk: clock, data flip: sint<42>>
// CHECK: %myinst_mem_read = firrtl.mem Undefined {depth = 1 : i64, name = "myinst_mem", portNames = ["read"], readLatency = 0 : i32, writeLatency = 1 : i32} : !firrtl.bundle<addr: uint<1>, en: uint<1>, clk: clock, data flip: sint<42>>
%mem_read = firrtl.mem Undefined {depth = 1 : i64, name = "mem", portNames = ["read"], readLatency = 0 : i32, writeLatency = 1 : i32} : !firrtl.bundle<addr: uint<1>, en: uint<1>, clk: clock, data flip: sint<42>>
// CHECK: %memoryport_data, %memoryport_port = chirrtl.memoryport Read %cmem {name = "memoryport"} : (!chirrtl.cmemory<uint<8>, 8>) -> (!firrtl.uint<8>, !chirrtl.cmemoryport)
// CHECK: %myinst_memoryport_data, %myinst_memoryport_port = chirrtl.memoryport Read %myinst_cmem {name = "myinst_memoryport"} : (!chirrtl.cmemory<uint<8>, 8>) -> (!firrtl.uint<8>, !chirrtl.cmemoryport)
%memoryport_data, %memoryport_port = chirrtl.memoryport Read %cmem {name = "memoryport"} : (!chirrtl.cmemory<uint<8>, 8>) -> (!firrtl.uint<8>, !chirrtl.cmemoryport)
chirrtl.memoryport.access %memoryport_port[%u8], %clock : !chirrtl.cmemoryport, !firrtl.uint<8>, !firrtl.clock
// CHECK: %myinst_node = firrtl.node %myinst_u8 : !firrtl.uint<8>
Expand All @@ -269,7 +269,7 @@ firrtl.module @declarations(in %clock : !firrtl.clock, in %u8 : !firrtl.uint<8>,
%reg = firrtl.reg %clock {name = "reg"} : !firrtl.clock, !firrtl.uint<8>
// CHECK: %myinst_regreset = firrtl.regreset %myinst_clock, %myinst_reset, %c0_ui8 : !firrtl.clock, !firrtl.asyncreset, !firrtl.uint<8>, !firrtl.uint<8>
%regreset = firrtl.regreset %clock, %reset, %c0_ui8 : !firrtl.clock, !firrtl.asyncreset, !firrtl.uint<8>, !firrtl.uint<8>
// CHECK: %smem = chirrtl.seqmem Undefined : !chirrtl.cmemory<uint<8>, 8>
// CHECK: %myinst_smem = chirrtl.seqmem Undefined : !chirrtl.cmemory<uint<8>, 8>
%smem = chirrtl.seqmem Undefined : !chirrtl.cmemory<uint<8>, 8>
// CHECK: %myinst_wire = firrtl.wire : !firrtl.uint<1>
%wire = firrtl.wire : !firrtl.uint<1>
Expand Down

0 comments on commit 917cde6

Please sign in to comment.