Add SuggestSeqMemName annotation to rename replaced blackboxes

[jared-barocsi]

This PR implements an annotation (SuggestSeqMemNameAnnotation) to rename a memory blackbox to a user-specified name instead of the default {mem name}_ext.

Contributor Checklist

• Did you add Scaladoc to every public function/method?
• Did you update the FIRRTL spec to include every new feature/behavior?
• Did you add at least one test demonstrating the PR?
• Did you delete any extraneous printlns/debugging code?
• Did you specify the type of improvement?
• Did you state the API impact?
• Did you specify the code generation impact?
• Did you request a desired merge strategy?
• Did you add text to be included in the Release Notes for this change?

Type of Improvement

• new feature/API

API Impact

No new API

Backend Code Generation Impact

Memory blackboxes get renamed with th

Desired Merge Strategy

Squash and merge

Release Notes

Implement a mechanism to rename blackboxes generated by ReplSeqMems with a user-specified name

Reviewer Checklist (only modified by reviewer)

• Did you add the appropriate labels?
• Did you mark the proper milestone (1.2.x, 1.3.0, 1.4.0) ?
• Did you review?
• Did you check whether all relevant Contributor checkboxes have been checked?
• Did you mark as Please Merge?

[jackkoenig]

This looks great! Just a couple of minor nits

[seldridge]

As an alternative approach... would it make sense to just keep the user-provided name of the memory for the blackbox and change the name of the blackbox wrapper module that gets generated?

[mwachs5]

does this change the instance names as well as the module names?

[jackkoenig]

As an alternative approach... would it make sense to just keep the user-provided name of the memory for the blackbox and change the name of the blackbox that gets generated?

@seldridge can you clarify this? You referred to the name of the blackbox twice.

does this change the instance names as well as the module names?

@mwachs5 great question, @jared-barocsi see my new comment on the test

The main goal here is that we want to be able to prefix the names of the modules that are creates by ReplSeqMems, it also seemed like some user control over the naming made sense. We want this to compose with a built-in prefixing API in Chisel3 so maybe some feedback in that context would be helpful as well.

[jackkoenig]

This test is showing the memory name in MemConf, but it would be really helpful if it could also show what the instance and module names are for the wrapper module, as well as the instance of the blackbox.

[mwachs5]

is the goal to change the instance name? I dont think in general we want to, but what it was doing before that the instance name was the same as the blackbox name... which IMHO is strange but that's the existing behavior

[jackkoenig]

I would say we do not want to change the instance name

[mwachs5]

I guess by "change" there is "same as it was before this PR" or "change based on what the person passes in"... which do you mean we do NOT want to do

[seldridge]

@jackkoenig wrote:

@seldridge can you clarify this? You referred to the name of the blackbox twice.

Ack. Fixed above.

It seems like a cleaner API is to have the memory blackbox inherit the name of the original memory (foo not foo_ext) and to instead mangle the name of the wrapping module (e.g., use foo_wrapper instead of foo). Then Chisel is in complete control of the final memory name and there is not a need for another API to control the specific naming behavior of a specific, optional pass.

[jackkoenig]

It seems like a cleaner API is to have the memory blackbox inherit the name of the original memory (foo not foo_ext) and to instead mangle the name of the wrapping module (e.g., use foo_wrapper instead of foo). Then Chisel is in complete control of the final memory name and there is not a need for another API to control the specific naming behavior of a specific, optional pass.

I think this is a reasonable suggestion, but it either doesn't fulfill the intended use case or would be a bit weird. The Prefixing API needs to make sure that all modules use the prefix, so this includes both the wrapper module and the blackbox. Using your suggestion, that means Chisel would have to give the prefix to the instance name of the memory which is a bit odd, I think that should still be derived from the name of the val. That or with your suggestion, users who want prefixing to affect repl seq mem would have to wrap every SyncReadMem in basically prefix(Module.currentModulePrefix) which wouldn't really work when trying to reuse other people's code (eg. Queue).

This is, of course, dancing around the need for packages/namespaces, and it's basically a huge pain that we can prefix all modules in Chisel (via chipsalliance/chisel#2155) but then ReplSeqMems decides to just screw everything up.

[mwachs5]

that means Chisel would have to give the prefix to the instance name of the memory which is a bit odd,

We should NOT give the prefix to the instance name of the memory.