This is opening an interesting door though...now that Chisel can support such information, what APIs merit having Objects built in within Chisel? Should this always be added in SRAM, or maybe there is a subclass or some other class that users can pick if they want the description.

In my team, I was proposing:

• for each module, they should be serializable module and put all parameters and main class to Property, so we are planing provide each module the Serializable OM description;
• power information: register that need to be associated with Path, it will be collected and generate the upf;
• IP integration: interface and memory region definition that required by IP-XACT;

To fully replace the existing CIRCT pass that's already adding basically this information, we'd need to work out all the weird uses downstream. Specifically, there is a behavior in CIRCT that the hierarchy field is the hierarchical path before any SRAM extraction has happened, whereas if this is implemented in Chisel, it will always point to the final hierarchical path of the SRAM. That's arguably the right thing, but I know of downstream users who depend on the previous, non-intuitive behavior.

I do agree we should replace the CreateSiFiveMetadata pass, and because the name, I believe SiFive is the only downstream to that pass. So maybe migration should be simple?

cc @debs-sifive and @jackkoenig, I reverted #4364 in a8cdaf4: since I think exposing the underlying API to users is not a good idea, and may introduce some inconvenience for future API change. I added 09254b9 instead with a private API and only used by the OM feature. For any users need the SRAM path, they can always retrieve the the Path via OM API in CIRCT.
WDYT?

@sequencer thanks for looking at this — I think your approach is definitely safer and more user-friendly than using underlying! There's some discussion on our end about being able to see hierarchy, which is the field I need. I'm copy-pasting @albertchen-sifive

could we make SRAMDescription a bundle with just property fields instead of an Class? I don’t think we have anyway of doing a field lookup like Property[SRAMDescription].hierarchy for properties? but if SRAMDescription was a Bundle and we had val description: SRAMDescription in SRAMInterface then we could do sram.description.hierarchy

could we make SRAMDescription a bundle with just property fields instead of an Class? I don’t think we have anyway of doing a field lookup like Property[SRAMDescription].hierarchy for properties? but if SRAMDescription was a Bundle and we had val description: SRAMDescription in SRAMInterface then we could do sram.description.hierarchy

If we are construction another standalone OM, e.g. SomeCoreL1DCacheDescription: Class, and only need a Path, it does makes sense. I think this flow can only add Path to SomeCoreL1DCacheDescription.hierarchy field, and append other fields, and do post-process, this reasonable. My question is what's the difference of using hierarchy: Property[Path] or SRAMDescription: Class? Since they are always used in the CIRCT side.

My concern is: a Class is a property for the whole SRAM metadata, and we only use the description after firtool compiling mlirbc. And I personally hope using this metadata to replace whole repl-sram pass and the metadata folder from circt output for better SRAM replacing experience.
Basically Path or Class are both OK in the flow designing, and for a standard ABI, I personally prefer using Class for SRAM, any other downstream user can always use the SRAMDescription to do the SRAM replacement flow. e.g. add their own pass find and match the Class.

My question is what's the difference of using hierarchy: Property[Path] or SRAMDescription: Class? Since they are always used in the CIRCT side.

The problem is that there is currently no way in Chisel to access the fields of the Property[SRAMDescription]. I think this is an issue with our current Property API, not with this PR per se, but we need the specific Property[Path] from SRAMDescription in the short term. I'm going to open a PR with a test that shows the lacking API in Property[_ <: Class] to show what I mean and then we can tackle it.

Basically Path or Class are both OK in the flow designing, and for a standard ABI, I personally prefer using Class for SRAM,

It's not Class vs. Path, it's Class vs. Bundle of the same properties that the Class had since it is possible to access fields of a Bundle. I do agree that Class is better though and maybe we can just make that work, see my coming PR showing what I mean.

Marked API Modification because this will break any use of SRAMInterface where the user instantiates it themselves rather than via the SRAM factory methods. They will have to hook up the new description field.

The problem is that there is currently no way in Chisel to access the fields of the Property[SRAMDescription].

Yes, That's a problem, in our side, we just throw the entire Property to another Property rather than filter out some internal property LOL, the output Top OM might be verbosity though...

I pushed a commit showing what I want to see, I thought we could get by with .getField but that doesn't seem to be defined on Property[ClassType] which seems like an oversight. I think if the test I pushed can pass, we're in good shape.

I pushed a commit showing what I want to see, I thought we could get by with .getField but that doesn't seem to be defined on Property[ClassType] which seems like an oversight. I think if the test I pushed can pass, we're in good shape.

I see. this is another key feature I also need, T1 has a fat OM due to this limitation

Or possible the issue is that instances of classes aren't the right API, but perhaps it really should be a of Properties... I'm playing around with the PR I promised but it does feel weird that classes are Module-like but I'm basically trying to use them as a struct of properties. I think this is a question for @mikeurbach and @albertchen-sifive about what really is the intended usage model here.

it does feel weird that classes are Module-like

I was confused by this for a long time. But this is the only way I made it successfully work😅

I changed SRAMDescription to a Bundle and it seems to work better.

cc @jackkoenig, Bundle make OM API extensively tricky to design, thus I switch to Class instead, I believe we need more APIs for Class API. rather than using Bundle for storage.