Fix memory annotation deduplication #2286

jared-barocsi · 2021-06-30T20:34:37Z

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

Bug fix
Backend code generation

API Impact

No changes to any API

Backend Code Generation Impact

MemoryInitAnnotations and its subclasses are 're-localized' after module deduplication, which will result in the correct Verilog being emitted for memories with these annotations.

Desired Merge Strategy

Squash and merge

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?

seldridge

Awesome. Thanks for tackling this, @jared-barocsi!

I have some thoughts about the dependencies for this transform included inline.

seldridge · 2021-06-30T21:01:49Z

src/main/scala/firrtl/transforms/DedupAnnotations.scala

+  override def prerequisites = firrtl.stage.Forms.LowForm
+
+  override def optionalPrerequisites = Nil
+
+  override def optionalPrerequisiteOf = firrtl.stage.Forms.BackendEmitters


I think this actually has no prerequisites and no optionalPrerequisiteOf.

seldridge · 2021-06-30T21:06:00Z

src/main/scala/firrtl/stage/Forms.scala

@@ -84,7 +84,8 @@ object Forms {
      Dependency(passes.PadWidths),
      Dependency(passes.memlib.VerilogMemDelays),
      Dependency(passes.SplitExpressions),
-      Dependency[firrtl.transforms.LegalizeAndReductionsTransform]
+      Dependency[firrtl.transforms.LegalizeAndReductionsTransform],
+      Dependency[firrtl.transforms.DedupAnnotationsTransform]


I think this should actually be part of ChirrtlForm and EliminateTargetPaths invalidates it. Also, any transform which implements the ResolvedAnnotationPaths trait would now invalidate annotation deduplication.

Does that make sense to you?

Yes, that does make sense. I noticed that the actually deduplication passes themselves were in HighForm, but I was previously pointed to VerilogMinimumOptimized being where I would put this pass. Clearly that approach ends up breaking some tests, along with LowFormMinimumOptimized, so I'll try and get some more clarification on where it should belong.

Also, any change to Forms.scala will cause the LoweringCompilersSpec to break. That is a super-legacy thing that is hard-coding the transform order before the Dependency API was added. It then compares the new order, computed from the Dependency API, applies any "patches" to that order, and then checks that it matches. That test always needs to be updated if anything in Forms.scala changes or if dependencies change.

(It's expected that that will break with the change you made here! That will need a patch.)

It seems that adding DedupAnnotationsTransform to Deduped, which is then appended to HighForm, does not require a change to LoweringCompilersSpec

…dup annotations transform

jackkoenig

This is a good start, thanks for the working test!

I want to take this in a slightly different direction, my suggestion comment ended up getting really long so it might be helpful to meet to discuss it.

src/main/scala/firrtl/transforms/DedupAnnotations.scala

jackkoenig

This is really great, excellent work @jared-barocsi!

I left some comments inline and 1 more thing I'd like to see is the Verilog Emitter to error if the annotations were unable to be deduped. I think that logic would live here:

firrtl/src/main/scala/firrtl/backends/verilog/VerilogEmitter.scala

Line 465 in 1f7faaa

private[firrtl] class EmissionOptions(annotations: AnnotationSeq) {

src/main/scala/firrtl/Utils.scala

src/main/scala/firrtl/annotations/MemoryInitAnnotation.scala

jackkoenig · 2021-07-08T20:14:10Z

src/main/scala/firrtl/annotations/Annotation.scala

+    * Returning None signifies this annotation will not deduplicate.
+    * @return
+    */
+  private[firrtl] def dedup: Option[((ReferenceTarget, Any), Annotation, ReferenceTarget)] = None


Suggested change

private[firrtl] def dedup: Option[((ReferenceTarget, Any), Annotation, ReferenceTarget)] = None

private[firrtl] def dedup: Option[(Any, Annotation, ReferenceTarget)] = None

Since (ReferenceTarget, Any) is itself an Any, I think the key should just be Any.

Should this be private to firrtl? I may be wrong but it seems to me to be pretty important to annotation writers.

It would be useful to have a generic mixin that automagically defines this, by grabbing all the parameters of the case class and filtering out the target.

I totally agree this could be important, I don't want it to be public yet. I mainly want to get this fix in and released as 1.4.4 while we discuss what the user-facing API should be.

src/main/scala/firrtl/transforms/DedupAnnotations.scala

src/test/scala/firrtlTests/MemoryInitSpec.scala

…rtl into fix-memory-anno-dedups

…dedup

jackkoenig

LGTM! Great work!

* Add transform to deduplicate memory annotations * Add annotation deduplication to Dedup stage * ResolveAnnotationPaths and EliminateTargetPaths now invalidate the dedup annotations transform * Verilog emitter now throws exception when memory annotations fail to dedup Co-authored-by: Jack Koenig <koenig@sifive.com> (cherry picked from commit 4081d9f)

kammoh · 2021-07-16T06:48:34Z

The problem I reported here doesn't seem to be resolved.

AttributeAnnotation disappears when more than one instance of the module Ram1R1W exist:
https://scastie.scala-lang.org/epf6LnZkRNiXxq0vYbez4Q

@jackkoenig Do you think it could be a different issue?

jared-barocsi · 2021-07-19T22:54:44Z

@kammoh, AttributeAnnotation needs a deduplication implementation of its own. I can follow this up with a secondary PR to resolve the issue.

seldridge · 2021-07-19T23:08:35Z

Would you be able to provide a default implementation for all SingleTargetAnnotation (and possibly MultiTargetAnnotation)? That should catch most of this stuff.

jared-barocsi · 2021-07-19T23:19:51Z

Would you be able to provide a default implementation for all SingleTargetAnnotation (and possibly MultiTargetAnnotation)? That should catch most of this stuff.

I don't think this is possible because the tuple ('dedup key') that's used to sort all of the annotations into deduplicatable groups contains the annotation's value, and that is different across implementations of SingleTargetAnnotation (for example, MemoryFileInlineAnnotation has a filename, MemoryArrayInitAnnotation has an array values, AttributeAnnotation has the description string).

MultiTargetAnnotation requires an additional patch to the transform so that it can correctly check each target against the circuit's InstanceGraph.

seldridge · 2021-07-20T01:59:04Z

🤔 I think what you may need here is to define "annotation value equality". Given both the existing duplicate method (explaining how to copy the annotation with a new target) and a way to compare two annotations, ignoring their targets, you may be able to avoid the value issue. For SingleTargetAnnotation, I think "annotation value equality" could be derived as equality of all product members that are not the target. For other annotations, a user may need to define this.

I think you could then group all annotations that are "annotation value equal" and, for their targets, deduplicate those and generate new annotations. I.e., produce fewer annotations or annotations with simpler targets.

I'm just spitballing here and trying to reuse as much work as possible. 😄

What do you think?

I admit MultiTargetAnnotation is a pain, but is fortunately much rarer. No need for a solution there.

davidbiancolin · 2021-07-20T03:54:31Z

+1 for @seldridge's idea, but I think Jack was concerned about the API leaking out before a longer-term annotation deduplication strategy is worked out. At very least, it could be done for SingleTargetAnnotations and kept package private for now.

I'd also like see inline attributes fixed in the next 1.4.x release.

* Fix memory annotation deduplication (#2286) * Add transform to deduplicate memory annotations * Add annotation deduplication to Dedup stage * ResolveAnnotationPaths and EliminateTargetPaths now invalidate the dedup annotations transform * Verilog emitter now throws exception when memory annotations fail to dedup Co-authored-by: Jack Koenig <koenig@sifive.com> (cherry picked from commit 4081d9f) * Fix binary compatibility issues with memory anno dedup transform (#2295) * Remove unused exception in dedup transform * Fold Annotation.dedup() into dedup transform for binary compatibility * ResolvedAnnotationPaths no longer invalidates any transforms * Use Annotation.copy() instead of constructor in dedup logic Co-authored-by: Jared Barocsi <82000041+jared-barocsi@users.noreply.github.com>

jardhu added 3 commits June 30, 2021 13:16

Add transform to deduplicate memory annotations

178faaa

Add unit test for annotation dedup transform

297226d

Remove print statement from dedup test

babcc14

seldridge reviewed Jun 30, 2021

View reviewed changes

jared-barocsi force-pushed the fix-memory-anno-dedups branch from bca5359 to c873e99 Compare June 30, 2021 21:18

Add annotation deduplication to Dedup stage

80c9f71

jared-barocsi force-pushed the fix-memory-anno-dedups branch from c873e99 to 80c9f71 Compare June 30, 2021 22:06

jardhu added 2 commits June 30, 2021 16:03

Remove DedupAnnotationsTransform's dependencies

55f4cf6

ResolveAnnotationPaths and EliminateTargetPaths now invalidate the de…

2c036f8

…dup annotations transform

jackkoenig reviewed Jul 1, 2021

View reviewed changes

src/main/scala/firrtl/transforms/DedupAnnotations.scala Outdated Show resolved Hide resolved

src/main/scala/firrtl/transforms/DedupAnnotations.scala Outdated Show resolved Hide resolved

jardhu and others added 4 commits July 6, 2021 17:01

Refactor the anno deduplication algorithm

3b35a61

Fix type mismatch compilation error

7d2b233

Fix some scaladoc issues

5b0bb72

Merge branch 'master' into fix-memory-anno-dedups

09c860d

jackkoenig suggested changes Jul 8, 2021

View reviewed changes

jardhu added 8 commits July 9, 2021 12:14

Use pathlessTarget instead of util func

d35a946

Memory init keeps dedup overrides private

e017773

Private the instancepath utility type

cf0d5f9

Merge branch 'fix-memory-anno-dedups' of github.com:jared-barocsi/fir…

55c9f57

…rtl into fix-memory-anno-dedups

Implement unit tests for cases when annotations would not deduplicate

d3ecbda

Formatting issues in annotation source

4e651af

Verilog emitter now throws exception when memory annotations fail to …

67a0b7a

…dedup

Compilation bug fix

bd87ee6

jared-barocsi marked this pull request as ready for review July 13, 2021 18:59

jardhu and others added 3 commits July 13, 2021 13:02

Scala doc for pathless target

ac04955

Merge branch 'master' into fix-memory-anno-dedups

ba5441c

Update pathlessTarget ScalaDoc

ac50406

jackkoenig approved these changes Jul 14, 2021

View reviewed changes

jackkoenig added this to the 1.4.x milestone Jul 14, 2021

jackkoenig merged commit 4081d9f into chipsalliance:master Jul 14, 2021

mergify bot mentioned this pull request Jul 14, 2021

Fix memory annotation deduplication (backport #2286) #2294

Merged

mergify bot added the Backported This PR has been backported to marked stable branch label Jul 14, 2021

This was referenced Jul 14, 2021

Module with multiple memories have no readmem statements #2168

Open

Verilog attribute annotations in modules are being lost when dedupping chipsalliance/chisel#1846

Open

jared-barocsi mentioned this pull request Jul 16, 2021

Fix binary compatibility issues with memory anno dedup transform #2295

Merged

14 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix memory annotation deduplication #2286

Fix memory annotation deduplication #2286

jared-barocsi commented Jun 30, 2021

seldridge left a comment

seldridge Jun 30, 2021

seldridge Jun 30, 2021

jared-barocsi Jun 30, 2021

seldridge Jun 30, 2021 •

edited

Loading

jared-barocsi Jun 30, 2021 •

edited

Loading

jackkoenig left a comment

jackkoenig left a comment

jackkoenig Jul 8, 2021

davidbiancolin Jul 8, 2021

jackkoenig Jul 9, 2021 •

edited

Loading

jackkoenig left a comment

kammoh commented Jul 16, 2021

jared-barocsi commented Jul 19, 2021

seldridge commented Jul 19, 2021

jared-barocsi commented Jul 19, 2021 •

edited

Loading

seldridge commented Jul 20, 2021

davidbiancolin commented Jul 20, 2021

	private[firrtl] def dedup: Option[((ReferenceTarget, Any), Annotation, ReferenceTarget)] = None
	private[firrtl] def dedup: Option[(Any, Annotation, ReferenceTarget)] = None

Fix memory annotation deduplication #2286

Fix memory annotation deduplication #2286

Conversation

jared-barocsi commented Jun 30, 2021

Contributor Checklist

Type of Improvement

API Impact

Backend Code Generation Impact

Desired Merge Strategy

Release Notes

Reviewer Checklist (only modified by reviewer)

seldridge left a comment

Choose a reason for hiding this comment

seldridge Jun 30, 2021

Choose a reason for hiding this comment

seldridge Jun 30, 2021

Choose a reason for hiding this comment

jared-barocsi Jun 30, 2021

Choose a reason for hiding this comment

seldridge Jun 30, 2021 • edited Loading

Choose a reason for hiding this comment

jared-barocsi Jun 30, 2021 • edited Loading

Choose a reason for hiding this comment

jackkoenig left a comment

Choose a reason for hiding this comment

jackkoenig left a comment

Choose a reason for hiding this comment

jackkoenig Jul 8, 2021

Choose a reason for hiding this comment

davidbiancolin Jul 8, 2021

Choose a reason for hiding this comment

jackkoenig Jul 9, 2021 • edited Loading

Choose a reason for hiding this comment

jackkoenig left a comment

Choose a reason for hiding this comment

kammoh commented Jul 16, 2021

jared-barocsi commented Jul 19, 2021

seldridge commented Jul 19, 2021

jared-barocsi commented Jul 19, 2021 • edited Loading

seldridge commented Jul 20, 2021

davidbiancolin commented Jul 20, 2021

seldridge Jun 30, 2021 •

edited

Loading

jared-barocsi Jun 30, 2021 •

edited

Loading

jackkoenig Jul 9, 2021 •

edited

Loading

jared-barocsi commented Jul 19, 2021 •

edited

Loading