Fix EliminateTargetPaths renaming

[albertchen-sifive]

There were a couple issues with the way targets were renamed. EliminateTargetPaths would only self-rename paths leading to the module of each duplicated IsModule. and foreach target it duplicated it only renamed relative paths starting from module. Don't know if I explained it very well, but hopefully the test cases make the issue a little clearer.

For the following circuit

         circuit Top:
           module Core:
             node clock = UInt<1>(0)
           module System:
             inst core_1 of Core
           module Top:
             inst system1 of System
             inst system2 of System
             inst system3 of System

resolving Top|Top/system1/core_1:Core and Top|Top/system2/core_1:Core would lead to the renames

Top|Top/system1/core_1:Core -> ~Top|Core___Top_system1_core_1
Top|Top/system2/core_1:Core -> ~Top|Core___Top_system2_core_1
Top|Top/system3/core_1:Core -> List(~Top|Top/system3:Core___Top_system1_core_1, ~Top|Top/system3:Core___Top_system2_core_1)

Top|Top/system3/core_1:Core should not be renamed to itself, but is instead renamed to the duplicated modules because self-renaming did not consider all paths leading to Core.

This PR updates the renaming to consider all paths leading to duplicated modules, including ones originating from above of module.

Also update the tests to use the updated DummyAnnotation which no longer extends SingleTargetAnnotation. this is to prevent potentially masking renames from single targets to multiple targets.

Contributor Checklist

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

Type of Improvement

• bug fix

API Impact

none

Backend Code Generation Impact

none

Desired Merge Strategy

• Squash

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]

Just to clarify: with the fix in this PR, the resulting renames do not include any renaming of the target that includes system3, i.e., there is now no renaming of Top|Top/system3/core_1:Core?

Great find. I'm happy to take a look at this (since I was hit with the round-robin assignment), but I won't be able to review until next week. Maybe @azidar can take a look sooner?

[albertchen-sifive]

Just to clarify: with the fix in this PR, the resulting renames do not include any renaming of the target that includes system3, i.e., there is now no renaming of Top|Top/system3/core_1:Core?

yeah Top|Top/system3/core_1:Core stays the same

[jackkoenig]

It appears that this is related to the issue in https://github.com/freechipsproject/firrtl/pull/1776#issuecomment-663698680, we have to rename all relative paths to a module in order to make the renaming right which is not great.

[seldridge]

Thanks for taking a look at this, @azidar.