Eager use of FIRRTL 1.4+ dedup can cause issues with bridge connectivity annotations

[albert-magyar]

So far, there has been a sequence of three successive workaround/fixes that have gradually moved closer to ideal FIRRTL dedup performance.

1. Turn off dedup entirely with a global firrtl.transforms.NoCircuitDedupAnnotation (breaks optimizations, slows compile times).
2. Use an in-Firesim copy of the FIRRTL 1.3 dedup transform (added in #668, enabled in #693, fixed in #699). This still required disabling 1.4+ dedup, and this strategy ultimately failed to deliver good dedup results.
3. Use a global firrtl.transforms.NoCircuitDedupAnnotation to prevent deduplication as part of default FIRRTL lowering stages but use a wrapper transform to allow finer-grained control of when dedup runs. This allows Golden Gate to rely on the upsteam FIRRTL dedup from releases 1.4+ (#738).

[timsnyder-siv]

#738 also removed the copied dedup and the comment in https://github.com/firesim/firesim/blob/9598f1efe9e838858b0b3d0fc66fe22f2471d486/sim/target-agnostic.mk#L75-L86

needs to be updated to correctly explain what is going on. Probably should be updated to say something like number 3 from Albert's comment above? It seems like this should maybe be done inside GoldenGate so that users don't have to remember to include --no-dedup, GG just adds the firrtl.transforms.NoCircuitdedupAnnotation and does the right thing without needing the cmdline option. Perhaps it already does that and we can just update target-agnostic.mk?

[albert-magyar]

the comment in [] needs to be updated to correctly explain what is going on. Probably should be updated to say something like number 3 from Albert's comment above? It seems like this should maybe be done inside GoldenGate so that users don't have to remember to include --no-dedup, GG just adds the firrtl.transforms.NoCircuitdedupAnnotation and does the right thing without needing the cmdline option.

Yes -- both sound like they would be positive changes. The "dedup-enabling logic" currently being split across two places (MidasTransforms and makefile) is less clear than it could be.

Perhaps it already does that and we can just update target-agnostic.mk?

Unless something has changed, it does not, so a CLI-independent addition of the annotation would need to be added.