[FIRRTL][OMIR] Update OMIR SRAM Paths #2024
Conversation
There was a problem hiding this comment.
This is coming together nicely! Some notes throughout.
One thing: this currently requires that the blackbox memory pass is being run. If this runs without blackbox memory, then it'll error out due to emitTrackedTarget not handling firrtl.mem: SRAM.scala:38:34: error: invalid target for "OMMemberReferenceTarget" OMIR. I think this is just a missing case there.
Additionally, if you run with blackbox memory, that appears to be dropping the tracker annotations and you wind up with OMDeleted in the output. 😬
|
I think I addressed most of the review comments. |
Extend the `EmitOMIR` pass to handle the conversion of relative targets in the `instancePath` field of `OMSRAM` nodes to absolute targets.
fabianschuiki
force-pushed
the
dev/fschuiki/omir-srams
branch
from
342c699
to
94a736b
Compare
|
I've updated a few things and added additional test cases to ensure the SRAM path expansion also works on |
| // expected-error @+4 {{OMIR node targets ambiguous component `mem`}} | ||
| // expected-note @+3 {{may refer to the following paths:}} | ||
| // expected-note @+2 {{- $root/sub1:Submodule}} | ||
| // expected-note @+1 {{- $root/sub2:Submodule}} |
| NamedAttrList fields; | ||
| fields.append("id", | ||
| IntegerAttr::get(IntegerType::get(ctx, 64), annotationID++)); | ||
| fields.append("omir.tracker", UnitAttr::get(ctx)); | ||
| fields.append("path", StringAttr::get(ctx, path)); |
There was a problem hiding this comment.
Good thought to include this for error reporting later.
|
Thanks for the reviews @seldridge. Going to merge this now. @prithayan has a follow-up PR adding a command line option to |
This PR converts the SRAM paths in the OMIR from local to absolute paths.
OMIRannotation has the fieldOMString:OMSRAM, then it is a candidate for path replacement.FIRRTLnode, with the same tracker id.NonLocalAnchor.NonLocalAnchorto theSRAMnode.firrtl.nlaafter processing.