Memlib Refactor

[sequencer]

This PR depends on #2199
PLCT intern @sinofp is working on rewriting vlsi_mem_gen into a Firrtl Transform.
This PR refactor Memlib to pave the path for him to add Annotations for memlib.

1. remove -c option in --replSeqMem console output(this not used anymore in firrtl codebase)
2. replace ConfWriter with MemLibOutConfigFileAnnotation, ConfWriter should be removed in next FIRRTL verision, since it's not private.
3. deprecate CreateMemoryAnnotation, inline which in ReplSeqMem.
4. fix ReplSeqMemTests, checkMemConf will read from correspond annotations.

Generally this PR try to replace

firrtl/src/main/scala/firrtl/passes/memlib/ReplaceMemTransform.scala

Lines 135 to 145 in bf1cf3d

	def transforms(inConfigFile: Option[YamlFileReader], outConfigFile: ConfWriter): Seq[Transform] =
	Seq(
	new SimpleMidTransform(Legalize),
	new SimpleMidTransform(ToMemIR),
	new SimpleMidTransform(ResolveMaskGranularity),
	new SimpleMidTransform(RenameAnnotatedMemoryPorts),
	new ResolveMemoryReference,
	new CreateMemoryAnnotations(inConfigFile),
	new ReplaceMemMacros(outConfigFile),
	new WiringTransform
	)

with Annotation, to make memlib transforms be able to digest Annotation and generate another Annotation, rather than directly write the conf file.

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

• code refactoring

API Impact

Deprecate CreateMemoryAnnotations, ConfWriter

Backend Code Generation Impact

None

Desired Merge Strategy

• Rebase: You will rebase the PR onto master and it will be merged with a merge commit.

Release Notes

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 shouldn't be public, and also must be cleared each time the transform is run. Ideally we wouldn't have a class field for this information (arguments are better), but that obviously would be a much more intrusive change.

[sequencer]

I use this to replace writer, I think it is the cheapest patch version. ;p
I will make it private.

[ekiwi]

This shouldn't be public, and also must be cleared each time the transform is run.

Are you telling me that if my Transform is a class, only a single instance is created and could be used for multiple calls to execute? If that is the case, shouldn't all transforms be object instead of class?

[seldridge]

Yes, the dependency manager will only construct one instance of a class if it needs to run multiple times. Maybe this wasn't the best idea, but the intent was to reuse existing transforms to solve lowerings due to invalidations. This can be a source of bugs (like this) where there is state in the transform.

[sequencer]

Got it. So here are two options:

1. remove defAnnotatedMemories, but which might introduce a bigger diff.
2. clean up defAnnotatedMemories at last of this transform.

Which one should I adopt? I personally prefer using the second one currently, and open another PR for dependency API until we get vlsi_mem_gen into firrtl.

[ekiwi]

Maybe this wasn't the best idea, but the intent was to reuse existing transforms to solve lowerings due to invalidations.

I see. I guess that would mean that eventually we just migrate all transforms to be object and then maybe we can deprecate Dependency[..] and leave Dependency(...) as the only option.

(Note: sorry for hijacking this thread)

[seldridge]

(2) may also require defining a critical region to serialize accesses to the mutable internals. I think there can be bugs when working in a parallel environment using the same stage. This can happen in tests.

@ekiwi: That seems very reasonable, though a long, painful deprecation road. :)

[sequencer]

(2) may also require defining a critical region to serialize accesses to the mutable internals. I think there can be bugs when working in a parallel environment using the same stage. This can happen in tests.

Got it, I will directly refactor it ;)

(Note: sorry for hijacking this thread)

Haha, no worries, I think this did help me understand what I should do for this PR.

[sequencer]

ready for review again: 6ac90a3

[jackkoenig]

I like the overall idea, would like some clarification about the plan: is this just slated for 1.5 in which case we can backport some deprecations and then break APIs or should we instead work on making things binary compatible (honestly, doesn't seem worth it)?

I also really don't like the mutability of MemLibOutConfigFileAnnotation

[jackkoenig]

Why make this a trait (especially unsealed) rather than just methods on the class?

More problematic, I don't love the mutability here. Why not have 2 different annotations (shared functionality in a trait perhaps 😉), 1 before the information is evaluated and 1 after?

[sequencer]

I removed this API and added private[memlib] case class AnnotatedMemoriesCollectorAnnotation() extends NoTargetAnnotation, I don't like mutable neither, but I think in this transform we must have a mutable thing to store those DefAnnotatedMemory, during walking the entire circuit: originally it was the config writer, in b385574, it was HasAnnotatedMemories, and in 7f82ea1, it is AnnotatedMemoriesCollectorAnnotation, I use insert and done function to make sure it only can be written during walking the circuit at first time, so I think it's safe here.

[sequencer]

So generally I'd like to make a binary incompatible change in this PR, since memlib is pretty old, most of its API are public and too hard to maintain the compatibility here.
I'll make most of functions in these transforms private, and of cause make sure chipyard is not using those APIs.
After finishing those binary incompatible changes, I'll submit for review, and PR another deprecation only PR.

More problematic, I don't love the mutability here. Why not have 2 different annotations (shared functionality in a trait perhaps wink), 1 before the information is evaluated and 1 after?

I'll create 2 annotations, one annotation for collect DefAnnotatedMemory with a ListBuffer, and another annotation with another transform to covert this annotation to annotations which will emit files.

[sequencer]

@jackkoenig Need review again :)
After you think modifications are ok, I will start to add private keyword to make make those functions as private as possible.

[jackkoenig]

I think these methods should be private (since we're changing them anyway, might as well take the opportunity).

Also, rather than using insertFunc to mutate an Annotation, why not just pass a ListBuffer[DefAnnotatedMemory] here and get rid of AnnotatedMemoriesCollectorAnnotation altogether? It's all private, I don't think there's any need for that annotation when we could just use the mutable collection directly and construct MemLibOutConfigFileAnnotation at the end.

[sequencer]

get rid of AnnotatedMemoriesCollectorAnnotation

I personal prefer using an standalone Annotation to store this mutable data,so in the DumpMemoryAnnotations Transform, we can directly convert this annotation to other annotations, in current case only the configure file, in the next PRs, InlinedBlackBoxes can be converted to.
That’s the reason why I add this intermediate annotation.

[sequencer]

And another question is if we use mutable collection here, when I switch to dependency API, will it be thread safe? As @seldridge said,

I think there can be bugs when working in a parallel environment using the same stage. This can happen in tests.

this is the reason I put mutable thing in annotations, which makes transform fully immutable.

[sequencer]

I think these methods should be private (since we're changing them anyway, might as well take the opportunity).

Sure, I’ll add private keywords when finishing review for a easy rebasing flow.

[ekiwi]

And another question is if we use mutable collection here, when I switch to dependency API, will it be thread safe?

You just need to create one instance of the collection for every call to the execute function. Like do not make it a member of the class, instead create it in execute and give a reference to it to every function that needs to mutate.

[sequencer]

Got it, fixed :)

[sequencer]

Hi, @jackkoenig

Also, rather than using insertFunc to mutate an Annotation, why not just pass a ListBuffer[DefAnnotatedMemory] here and get rid of AnnotatedMemoriesCollectorAnnotation altogether?

Done. I have removed that annotation. But still have the AnnotatedMemoriesAnnotation For transform to other annotations. Is that ok? As @ekiwi suggested, mutable is in execute function now.

It's all private, I don't think there's any need for that annotation when we could just use the mutable collection directly and construct MemLibOutConfigFileAnnotation at the end.

Since after this PR, I wanna transform to Verilog blackboxes annotations, thus create the intermediate annotation, and consume it in next transform.

[mwachs5]

@sequencer , the behavior after this change is that ReplSeqMem removes the ReplSeqMemAnnotations (replacing them with MemLibOutConfigFileAnnotation). Is this intentional? If not we should fix it. If so, we should really put somethign in the release notes to this effect as its a pretty breaking change for anyone using ReplSeqMemAnnotations for other purposes

[sequencer]

I thought ReplSeqMemAnnotations is not removed? I kept it unchanged, ReplSeqMemAnnotations will be convert to MemLibOutConfigFileAnnotation in CreateMemoryAnnotations pass. What's the different behavior?

[sequencer]

btw, XiangShan projects has switched to new memlib pass in this PR: https://github.com/OpenXiangShan/XiangShan/pull/1000/files, hope this helps