[FIRRTL][RTL][ExportedVerilog] Missing name in black box memory 'rtl.instance' op requires attribute 'instanceName'

[JuanEsco063]

I am trying to generate RTL for a simple matmul kernel considering memories as black boxes with read and write latency of 1 after #493 and #585 (and modifying the corresponding files to include the changes of #602 ).

After running

circt-opt matmul_std_lin.mlir -create-dataflow -handshake-insert-buffer -lower-handshake-to-firrtl > matmul_firrtl_1.mlir

To generate the FIRRTL dialect file. Then I modify the aforementioned file to include a read latency of 1 and replace memories for their black box equivalent by running:

circt-opt matmul_firrtl_1.mlir -firrtl-blackbox-memory > matmul_firrtl_2.mlir

But when I try to generate Verilog using:

firtool -format=mlir matmul_firrtl_2.mlir -lower-to-rtl -enable-lower-types -verilog > new_matmul.v

I get the error:

matmul_firrtl_2.mlir:5:65: error: 'rtl.instance' op requires attribute 'instanceName'

I see @mikeurbach encountered a similar issue in #463 where he closed the case (based on #407 and his merge in #485) but now I am facing the same issue.

See attached files for reference.

MatmulFiles.zip

[youngar]

Hi! Thanks for reporting this, I just starting looking into it.

1. Despite what is said in [FIRRTL][RTL][ExportVerilog] Instances with no names #463, it looks like we never started supporting instances with no names
2. I can modify the memory blackboxing to add instance names based on the memory name, circumventing 1
3. After that, there is a problem with blackboxing write memories

I implemented 2 and 3 locally, which I will open PRs for. In the mean time, I've attached the final verilog.

edit: #674

new_matmul.v.zip

[mikeurbach]

Despite what is said in #463, it looks like we never started supporting instances with no names

I think it is important to differentiate between no instanceName attribute, and an instanceName attribute set to "". I believe the former still leads to this error, but the latter should lead to an auto-generated name for the instance in Verilog output.

The change looks good to me in #674, but for the record, what I mean is if this line was a StringAttr with an empty string instead of a null StringAttr, that should have worked.

But implementing 2. sounds like what we really want, thanks @youngar.

[JuanEsco063]

#674 is merged, solving this issue.

[youngar]

@JuanEsco063 Small heads up that the "emit-wrapper=false" option for blackboxing memories is not able to lower all the way to verilog yet, we need to support wires with bundle types in firrtl-lower-types.

StringAttr with an empty string instead of a null StringAttr, that should have worked.

That is helpful, I didn't realize this. I saw that the instanceName was Optional<> in FIRRTL, but not in RTL. Would it make sense to make this attribute optional in RTL, or at handle this difference in LowerToRTL?

[mikeurbach]

Would it make sense to make this attribute optional in RTL, or at handle this difference in LowerToRTL?

I think that was discussed in #463 (comment). IMHO that section of LowerToRTL you linked to could potentially have an else branch that created an instanceName attr with "" for the value to appease the verifier.

we need to support wires with bundle types in firrtl-lower-types.

Do you mean memories with bundle typed values? Most of what firrtl-lower-types does is lower wires with bundle types, and that already works for the Handshake use-case, which doesn't ever put bundles into memories.

[youngar]

I meant wires with bundle types, I didn't see support - maybe I'm missing the place? I started adding support here (WIP warning), but I think its lowering the connect statement wrong when there is a duplex type.

[mikeurbach]

Sorry, I saw #680 and now I understand.