[FIRRTL] Fix SRAM instance paths in OMIR

[fabianschuiki]

Fix an issue in EmitOMIR where the instance paths of SRAMs would be incorrectly rendered as OMMemberReferenceTarget instead of the correct OMMemberInstanceTarget. This also includes figuring out the module name that the final SRAM is going to have, which in the case of a firrtl.mem is a bit tricky/fragile: the actual module name for the memory is only generated after lowering to the HW dialect, at which point the OMIR has long been emitted. This change "predicts" the name that the memory is going to have by querying getFirMemoryName(), which looks like it's going to be accurate in most cases we currently care about.

In the long run, we'll want better ways to encode this formulation of an instance path and ideally dealy it until ExportVerilog time.

[prithayan]

Looks good

[seldridge]

Nice.

I'm annoyed that we don't have a good answer for how to update this during the memory replacement and have to (seemingly) pick up the pieces after it happens. I.e., is it possible (or a good idea) to update the target at the conversion from memory to instance?

This is clearly not necessary, but I think it's doable once we move away from OMIR encoded as dictionaries to whatever plethora of OMIR dialects exist. E.g., this could be represented as a canonicalization if we an actual op, in a separate dialect, for representing OMMemberReferenceTarget that morphs into an OMMemberInstanceTarget if the type of the thing with the symbol that it's pointing at changes.

[seldridge]

This lambda is clever (I hadn't seen this syntax before...), but is it necessary? 😉

[fabianschuiki]

😃 The trouble is that the ">componentName" case needs to run if either type != "OMMemberInstanceTarget" or none of the if statements runs. And since you you cannot check type == ... in the same if condition as you do auto instOp = ... you cannot fold the outer if into the inner ones. So either you need to

• have a bool to track whether any of the ifs above produced a result
• have ">componentName" output twice, once as else for the outer and once for the inner if statement
• have a closure that allows for early return

[fabianschuiki]

@seldridge I'm annoyed that we don't have a good answer for how to update this during the memory replacement and have to (seemingly) pick up the pieces after it happens. I.e., is it possible (or a good idea) to update the target at the conversion from memory to instance?

The trouble is that the conversion to an instance happens only in LowerToHW. So we'd only know what exactly to put here after lowering is done (and a symbol for the instantiated module has been created). I think what's missing is a way to encode "put the OMIR-formatted path of @someMagicInstance here" in the IR after lowering to HW. Then you wouldn't have to guess about the nature of that symbol (memory, instance, module, whatever), but could just rely on something late in the pipeline filling in the correct path. That could be a separate OMIR dialect, or an auxiliary operation whose result you could stick into sv.verbatim.

This is clearly not necessary, but I think it's doable once we move away from OMIR encoded as dictionaries to whatever plethora of OMIR dialects exist. E.g., this could be represented as a canonicalization if we an actual op, in a separate dialect, for representing OMMemberReferenceTarget that morphs into an OMMemberInstanceTarget if the type of the thing with the symbol that it's pointing at changes.

Yes once we have a more fleshed-out dialect to do this tracking of structured data this should become easier. But especially the distinction of the different OMIR targets seems to be an artifact of the Scala code encoding the nature of the target in the type system, which is great on the Scala side. But on the MLIR side, all you actually need is a symbol for the target itself, and the type of target is fairly evident from what kind of operation it's attached to (module, instance, wire, reg, mem, etc.). I'd expect an OMIR-inspired dialect to be more MLIR-idiomatic such that morphing these targets is unnecessary.