[FIRRTL][CreateSiFiveMetadata] Add memory read-under-write behavior to emitted metadata #8604

fzi-hielscher · 2025-06-26T14:23:17Z

This PR adds information on the read under write behavior of memories, as specified by the FIRRTL/CHIRRTL IR, to the emitted memory metadata records. This includes the textual repl-seq-mem-file, the seq_mem.json, and the object model IR. It should allow external tools to generate memories with the appropriate behavior.

In the JSON file, there is a new string attribute ruw_behavior that can take one of the following values : Undefined, New, Old.
In the textual file, Undefined behavior is elided to keep the format unchanged for the common case. Otherwise ruw New or ruw Old is appended to the memory's line.
In the object model it is added to the MemorySchema as a string-typed subfield called ruwBehavior. I've never dealt with the object model before and I don't really know how enums should be handled here. So, please tell me if I should change my approach.

To make this information available at the point of metadata emission, I had to add the ruw attribute to FMemModuleOp. Previously, this attribute was just an I32EnumAttr. When adding it to the FMemModuleOp's attribute dictionary this would become a generic i32 typed attribute, which did not seem desirable to me. So I replaced the RUWAttr with a RUWBehaviorAttr dialect attribute to give it an enum specific serialization (ruw = #firrtl<ruwbehavior Undefined>). Unfortunately, this creates a fair amount of change noise. I can break it out as a separate PR if desired.

fzi-hielscher · 2025-06-26T14:45:33Z

include/circt-c/Dialect/FIRRTL.h

@@ -292,7 +292,7 @@ MLIR_CAPI_EXPORTED MlirAttribute firrtlAttrGetParamDecl(MlirContext ctx,
 MLIR_CAPI_EXPORTED MlirAttribute firrtlAttrGetNameKind(MlirContext ctx,
                                                       FIRRTLNameKind nameKind);

-/// Creates a RUWAttr with the specified Read-Under-Write Behaviour.
+/// Creates a RUWBehaviorAttr with the specified Read-Under-Write Behavior.


I apologise to the British, but since the FIRRTL spec is written in AE I chose 'behavior'.

It's usually the Canadians you have to watch out for...

fzi-hielscher · 2025-06-26T14:48:37Z

include/circt/Dialect/FIRRTL/FIRRTLStructure.td

+      DenseBoolArrayAttr:$portDirections, ArrayRefAttr:$portLocations,
+      ArrayRefAttr:$portAnnotations, ArrayRefAttr:$portSymbols,
+      ArrayRefAttr:$portNames, ArrayRefAttr:$portTypes,
+      DefaultValuedAttr<RUWBehaviorAttr, "firrtl::RUWBehavior::Undefined">:$ruw,


Making RUWBehavior::Undefined the default reduces the impact of this change but might increase the chances of accidentally dropping specified behavior.

fzi-hielscher · 2025-06-26T14:53:10Z

lib/Dialect/FIRRTL/Transforms/CreateSiFiveMetadata.cpp

    seqMemConfStr = (StringRef(seqMemConfStr) + "name {{" + Twine(symId) +
                     "}} depth " + Twine(mem.getDepth()) + " width " +
-                     Twine(width) + " ports " + portStr + maskGranStr + "\n")
+                     Twine(width) + " ports " + portStr + maskGranStr +
+                     (mem.getRuw() == RUWBehavior::Undefined
+                          ? ""
+                          : " ruw " + stringifyRUWBehavior(mem.getRuw())) +
+                     "\n")


This is getting a bit out of hand. When I'll also add the MemoryInitAttr to the metadata, I'll probably just append the string instead of concatenating twines.

fzi-hielscher · 2025-06-26T14:58:15Z

lib/Dialect/FIRRTL/Transforms/LowerMemory.cpp

@@ -206,7 +206,8 @@ LowerMemoryPass::emitMemoryModule(MemOp op, const FirMemory &mem,
  auto moduleOp = b.create<FMemModuleOp>(
      mem.loc, moduleName, ports, mem.numReadPorts, mem.numWritePorts,
      mem.numReadWritePorts, mem.dataWidth, mem.maskBits, mem.readLatency,
-      mem.writeLatency, mem.depth);
+      mem.writeLatency, mem.depth,
+      *symbolizeRUWBehavior(uint32_t(mem.readUnderWrite)));


Not a huge fan of this myself, but it is just the inverse of line 92: *seq::symbolizeRUW(unsigned(op.getRuw())),

fzi-hielscher · 2025-06-26T15:15:54Z

test/firtool/memoryMetadataRenameFail.fir

@@ -116,13 +116,13 @@ circuit test:
 ; CHECK:      FILE "metadata/tb_seq_mems.json"
 ; CHECK:      [{"module_name":"memory","depth":16,"width":8,"masked":"true",
 ; CHECK-SAME: "read":"true","write":"true","readwrite":"false",
-; CHECK-SAME: "mask_granularity":8,"extra_ports":[],
+; CHECK-SAME: "ruw_behavior": "Undefined","mask_granularity":8,"extra_ports":[],


I'm not really sure what this test is supposed to do. Does it just check that the metatdata is not emitted without the --repl-seq-mem option?
At the moment it should always expectedly fail as the formatting has not been updated.

I'm not sure... @prithayan do you have any idea?

Unfortunately, I've no recollection of why this was added. I'll get rid of it in another PR.

prithayan · 2025-06-27T19:09:07Z

Thanks @fzi-hielscher for the PR, I'm also taking a look at it, will have the review by Monday.

seldridge

LGTM

I'll try to get a head start to figure out if this is going to break anything internally due to the format changes of some of these files. However, that is my problem.

seldridge · 2025-07-08T15:36:03Z

lib/Dialect/FIRRTL/Transforms/CreateSiFiveMetadata.cpp

@@ -314,7 +315,7 @@ struct ObjectModelIR {
      extraPortsList.push_back(extraPortsObj);
    }
    auto extraPorts = builderOM.create<ListCreateOp>(
-        memorySchemaClass.getPortType(22), extraPortsList);
+        memorySchemaClass.getPortType(24), extraPortsList);


Why did this bump by +2 when others only bumped by +1?

Because it crashed after I initially just increased it by one. 😉

The builder for the ClassOp creates both an input and an output port for each field. So the extraPorts port is shifted by two positions.

That's right, FIRRTL class, doesn't have the fields representation. It uses input and output ports to represent fields, hence the +2

seldridge · 2025-07-08T15:37:17Z

lib/Dialect/FIRRTL/Transforms/LowerMemory.cpp

@@ -206,7 +206,8 @@ LowerMemoryPass::emitMemoryModule(MemOp op, const FirMemory &mem,
  auto moduleOp = b.create<FMemModuleOp>(
      mem.loc, moduleName, ports, mem.numReadPorts, mem.numWritePorts,
      mem.numReadWritePorts, mem.dataWidth, mem.maskBits, mem.readLatency,
-      mem.writeLatency, mem.depth);
+      mem.writeLatency, mem.depth,
+      *symbolizeRUWBehavior(uint32_t(mem.readUnderWrite)));


Nit?

Suggested change

*symbolizeRUWBehavior(uint32_t(mem.readUnderWrite)));

*symbolizeRUWBehavior(static_cast(mem.readUnderWrite)));

seldridge · 2025-07-08T15:38:36Z

test/Dialect/FIRRTL/SFCTests/directories.fir

-; 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<!om.path>, %inDut_in: i1, %extraPorts_in: !om.list<!om.class.type<@ExtraPortsMemorySchema>>, %preExtInstName_in: !om.list<!om.string>) -> (name: !om.string, depth: !om.integer, width: !om.integer, maskBits: !om.integer, readPorts: !om.integer, writePorts: !om.integer, readwritePorts: !om.integer, writeLatency: !om.integer, readLatency: !om.integer, hierarchy: !om.list<!om.path>, inDut: i1, extraPorts: !om.list<!om.class.type<@ExtraPortsMemorySchema>>, preExtInstName: !om.list<!om.string>)
-; MLIR_OUT:    om.class.fields %name_in, %depth_in, %width_in, %maskBits_in, %readPorts_in, %writePorts_in, %readwritePorts_in, %writeLatency_in, %readLatency_in, %hierarchy_in, %inDut_in, %extraPorts_in, %preExtInstName_in : !om.string, !om.integer, !om.integer, !om.integer, !om.integer, !om.integer, !om.integer, !om.integer, !om.integer, !om.list<!om.path>, i1, !om.list<!om.class.type<@ExtraPortsMemorySchema>>, !om.list<!om.string>
-; MLIR_OUT:  om.class @MemoryMetadata(%basepath: !om.basepath) -> (foo_m_ext_field: !om.class.type<@MemorySchema>, bar_m_ext_field: !om.class.type<@MemorySchema>, baz_m_ext_field: !om.class.type<@MemorySchema>)
+; 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, %ruwBehavior_in: !om.string, %hierarchy_in: !om.list<!om.path>, %inDut_in: i1, %extraPorts_in: !om.list<!om.class.type<@ExtraPortsMemorySchema>>, %preExtInstName_in: !om.list<!om.string>)  -> (name: !om.string, depth: !om.integer, width: !om.integer, maskBits: !om.integer, readPorts: !om.integer, writePorts: !om.integer, readwritePorts: !om.integer, writeLatency: !om.integer, readLatency: !om.integer, ruwBehavior: !om.string, hierarchy: !om.list<!om.path>, inDut: i1, extraPorts: !om.list<!om.class.type<@ExtraPortsMemorySchema>>, preExtInstName: !om.list<!om.string>)
+; MLIR_OUT:    om.class.fields %name_in, %depth_in, %width_in, %maskBits_in, %readPorts_in, %writePorts_in, %readwritePorts_in, %writeLatency_in, %readLatency_in, %ruwBehavior_in, %hierarchy_in, %inDut_in, %extraPorts_in, %preExtInstName_in : !om.string, !om.integer, !om.integer, !om.integer, !om.integer, !om.integer, !om.integer, !om.integer, !om.integer, !om.string, !om.list<!om.path>, i1, !om.list<!om.class.type<@ExtraPortsMemorySchema>>, !om.list<!om.string>


Ugh... this is not great to work with. Somebody (not you, likely me) needs to go in and convert all of this to *-SAME checks to make this more manageable.

seldridge · 2025-07-08T15:39:15Z

test/Dialect/FIRRTL/emit-metadata.mlir

+//------------------------------------------------------------------ (1) OM Info
+// CHECK-LABEL:         firrtl.class @MemoryMetadata({{.*$}}
+// CHECK:                 %[[memoryObject:.+]] = firrtl.object @MemorySchema
+// CHECK:                 %[[#a:]] = firrtl.string "Old"
+// CHECK-NEXT:            %[[#ruw:]] = firrtl.object.subfield %[[memoryObject]][ruwBehavior_in]
+// CHECK-NEXT:            firrtl.propassign %[[#ruw]], %[[#a]]
+
+//-------------------------------------------------------------- (2) Memory JSON
+// CHECK-LABEL:         emit.file "metadata{{/|\\\\}}seq_mems.json"
+// CHECK-NEXT:            sv.verbatim "[
+// CHECK:                  \22ruw_behavior\22: \22Old\22
+
+//------------------------------------------------------- (3) Configuration File
+// CHECK-LABEL:         emit.file "mems.conf"
+// CHECK-NEXT:            sv.verbatim
+// CHECK-SAME:            ruw Old


Very nice test organization. 💯

seldridge · 2025-07-08T15:41:35Z

test/firtool/memoryLowering.fir

+  {
+    "class":"sifive.enterprise.firrtl.MarkDUTAnnotation",
+    "target": "test.test"
+  }


Nit: is the MarkDUTAnnotation necessary?

Ditto for the second split test in this file.

seldridge · 2025-07-08T15:45:26Z

test/firtool/memoryMetadataRenameFail.fir

@@ -116,13 +116,13 @@ circuit test:
 ; CHECK:      FILE "metadata/tb_seq_mems.json"
 ; CHECK:      [{"module_name":"memory","depth":16,"width":8,"masked":"true",
 ; CHECK-SAME: "read":"true","write":"true","readwrite":"false",
-; CHECK-SAME: "mask_granularity":8,"extra_ports":[],
+; CHECK-SAME: "ruw_behavior": "Undefined","mask_granularity":8,"extra_ports":[],


I'm not sure... @prithayan do you have any idea?

fzi-hielscher · 2025-07-08T18:37:36Z

Thanks, @seldridge. It's not a problem for me to delay this a bit more to make sure all known users are prepared. I've also lined up the PR for MemInitAttr on top of this one. So if something needs changing, ideally that should be supported too:
fzi-hielscher#1

The script we are using internally has been forked many years ago from the rocket-chip repo:
https://github.com/chipsalliance/rocket-chip/blob/master/scripts/vlsi_mem_gen
I do not know if this is actively being used anymore.

prithayan

Looks good. Hopefully will "just work" with outer internal flow. :)

prithayan · 2025-07-08T22:05:12Z

test/Dialect/FIRRTL/SFCTests/directories.fir

-; MLIR_OUT:    om.class.fields %name_in, %depth_in, %width_in, %maskBits_in, %readPorts_in, %writePorts_in, %readwritePorts_in, %writeLatency_in, %readLatency_in, %hierarchy_in, %inDut_in, %extraPorts_in, %preExtInstName_in : !om.string, !om.integer, !om.integer, !om.integer, !om.integer, !om.integer, !om.integer, !om.integer, !om.integer, !om.list<!om.path>, i1, !om.list<!om.class.type<@ExtraPortsMemorySchema>>, !om.list<!om.string>
-; MLIR_OUT:  om.class @MemoryMetadata(%basepath: !om.basepath) -> (foo_m_ext_field: !om.class.type<@MemorySchema>, bar_m_ext_field: !om.class.type<@MemorySchema>, baz_m_ext_field: !om.class.type<@MemorySchema>)
+; 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, %ruwBehavior_in: !om.string, %hierarchy_in: !om.list<!om.path>, %inDut_in: i1, %extraPorts_in: !om.list<!om.class.type<@ExtraPortsMemorySchema>>, %preExtInstName_in: !om.list<!om.string>)  -> (name: !om.string, depth: !om.integer, width: !om.integer, maskBits: !om.integer, readPorts: !om.integer, writePorts: !om.integer, readwritePorts: !om.integer, writeLatency: !om.integer, readLatency: !om.integer, ruwBehavior: !om.string, hierarchy: !om.list<!om.path>, inDut: i1, extraPorts: !om.list<!om.class.type<@ExtraPortsMemorySchema>>, preExtInstName: !om.list<!om.string>)
+; MLIR_OUT:    om.class.fields %name_in, %depth_in, %width_in, %maskBits_in, %readPorts_in, %writePorts_in, %readwritePorts_in, %writeLatency_in, %readLatency_in, %ruwBehavior_in, %hierarchy_in, %inDut_in, %extraPorts_in, %preExtInstName_in : !om.string, !om.integer, !om.integer, !om.integer, !om.integer, !om.integer, !om.integer, !om.integer, !om.integer, !om.string, !om.list<!om.path>, i1, !om.list<!om.class.type<@ExtraPortsMemorySchema>>, !om.list<!om.string>


Most likely I'm the culprit, I'll format this properly in another PR.

prithayan · 2025-07-08T22:05:55Z

test/firtool/memoryMetadataRenameFail.fir

@@ -116,13 +116,13 @@ circuit test:
 ; CHECK:      FILE "metadata/tb_seq_mems.json"
 ; CHECK:      [{"module_name":"memory","depth":16,"width":8,"masked":"true",
 ; CHECK-SAME: "read":"true","write":"true","readwrite":"false",
-; CHECK-SAME: "mask_granularity":8,"extra_ports":[],
+; CHECK-SAME: "ruw_behavior": "Undefined","mask_granularity":8,"extra_ports":[],


Unfortunately, I've no recollection of why this was added. I'll get rid of it in another PR.

prithayan · 2025-07-08T22:09:18Z

lib/Dialect/FIRRTL/Transforms/CreateSiFiveMetadata.cpp

@@ -314,7 +315,7 @@ struct ObjectModelIR {
      extraPortsList.push_back(extraPortsObj);
    }
    auto extraPorts = builderOM.create<ListCreateOp>(
-        memorySchemaClass.getPortType(22), extraPortsList);
+        memorySchemaClass.getPortType(24), extraPortsList);


That's right, FIRRTL class, doesn't have the fields representation. It uses input and output ports to represent fields, hence the +2

fzi-hielscher added 4 commits June 26, 2025 00:54

[NFC][FIRRTL] Use a dialect attribute for read-under-write behavior

41b3a84

[FIRRTL] Add RUWBehavior annotation to FMemModuleOp

85ed3cd

[FIRRTL] Add memory read-under-write behavior to emitted metadata

79186e9

fixup! [FIRRTL] Add RUWBehavior annotation to FMemModuleOp

fed83c8

fzi-hielscher commented Jun 26, 2025

View reviewed changes

fzi-hielscher marked this pull request as ready for review June 26, 2025 15:17

fzi-hielscher requested review from darthscsi and seldridge as code owners June 26, 2025 15:17

fzi-hielscher requested review from mikeurbach and prithayan June 26, 2025 15:18

Undo clang-format changes to TableGen files

ecacb43

More focused test

c578a59

seldridge approved these changes Jul 8, 2025

View reviewed changes

Review comments

454f29a

prithayan approved these changes Jul 8, 2025

View reviewed changes

	*symbolizeRUWBehavior(uint32_t(mem.readUnderWrite)));
	*symbolizeRUWBehavior(static_cast(mem.readUnderWrite)));

[FIRRTL][CreateSiFiveMetadata] Add memory read-under-write behavior to emitted metadata #8604

Are you sure you want to change the base?

[FIRRTL][CreateSiFiveMetadata] Add memory read-under-write behavior to emitted metadata #8604

Conversation

fzi-hielscher commented Jun 26, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prithayan commented Jun 27, 2025

Uh oh!

seldridge left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fzi-hielscher commented Jul 8, 2025

Uh oh!

prithayan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!