[FIRRTL][Dedup] Remove unnecessary NLA creation for BlackBox annotations

[Tang-Haojin]

Hi, I am new to the CIRCT project. This is my first PR, and it may be incorrect. If there are any issues, please feel free to point them out.

The problem comes from the firrtl file:

FIRRTL version 6.0.0
circuit Foo :%[[
  {
    "class":"firrtl.transforms.DedupGroupAnnotation",
    "target":"~|BBox",
    "group":"BBox"
  },
  {
    "class":"firrtl.transforms.DedupGroupAnnotation",
    "target":"~|BBox_1",
    "group":"BBox"
  },
  {
    "class":"firrtl.transforms.DedupGroupAnnotation",
    "target":"~|Foo",
    "group":"Foo"
  },
  {
    "class":"firrtl.transforms.BlackBoxInlineAnno",
    "target":"~|BBox",
    "name":"BBox.sv",
    "text":""
  },
  {
    "class":"firrtl.transforms.BlackBoxInlineAnno",
    "target":"~|BBox_1",
    "name":"BBox.sv",
    "text":""
  }
]]
  layer Verification, bind, "verification" :
    layer Assert, bind, "verification/assert" :
    layer Assume, bind, "verification/assume" :
    layer Cover, bind, "verification/cover" :

  extmodule BBox :
    input in : UInt<8>
    output out : UInt<8>
    defname = BBox

  extmodule BBox_1 :
    input in : UInt<8>
    output out : UInt<8>
    defname = BBox

  public module Foo :
    input clock : Clock
    input reset : AsyncReset
    output io_bb : { flip in : UInt<8>, out : UInt<8>}
    output io_bb_2 : { flip in : UInt<8>, out : UInt<8>}

    inst bb of BBox
    connect bb.in, io_bb.in
    connect io_bb.out, bb.out
    inst bb2 of BBox_1
    connect bb2.in, io_bb_2.in
    connect io_bb_2.out, bb2.out

This firrtl file contains two extmodules, BBox and BBox_1, that are identical, which usually caused by calling Module(new BBox) twice in chisel. Both also carry the corresponding firrtl.transforms.BlackBoxInlineAnno. The generated HW dialect by firtool is:

module {
  hw.hierpath private @nla_0 [@Foo::@sym_0, @BBox]
  hw.hierpath private @nla [@Foo::@sym, @BBox]
  hw.module.extern private @BBox(in %in : i8, out out : i8) attributes {verilogName = "BBox"}
  hw.module @Foo(in %clock : !seq.clock, in %reset : i1, in %io_bb_in : i8, out io_bb_out : i8, in %io_bb_2_in : i8, out io_bb_2_out : i8) {
    %bb.out = hw.instance "bb" sym @sym @BBox(in: %io_bb_in: i8) -> (out: i8) {sv.namehint = "io_bb_out"}
    %bb2.out = hw.instance "bb2" sym @sym_0 @BBox(in: %io_bb_2_in: i8) -> (out: i8) {sv.namehint = "io_bb_2_out"}
    hw.output %bb.out, %bb2.out : i8, i8
  }
  emit.file "./BBox.sv" sym @blackbox_BBox.sv {
    emit.verbatim ""
  }
  om.class @Foo_Class(%basepath: !om.basepath) {
    om.class.fields 
  }
}

The output contain two unused (and may also be useless?) hw.hierpath operations plus the innerSym nla they reference. This PR aims to eliminate them.

In the copyAnnotations function, all annotations were being processed through makeAnnotationNonLocal by default. However, BlackBox annotations are specific to the external module itself and not needed to be made non-local during deduplication.

So here I Added special handling for BlackBoxInlineAnno and BlackBoxPathAnno in the copyAnnotations function, similar to the existing handling for DontTouchAnnotation.

[seldridge]

Generally, we've not added custom handling of annotations to the dedup modules pass (with the lone exception being DontTouchAnnotation). The reasoning for this is that no annotation is supposed to be treated differently from any other annotation. It's been viewed that allowing custom annotation handling would make things too complicated to reason about: certain annotations block deduplication, certain annotations dedup with each other, other annotations are "sticky" and will be applied if they dedup. Instead, all annotations preserve their information content by becoming non-local due to dedup (again, excepting DontTouchAnnotation).

What's really going on here is that BlackBoxReader is removing all users of these NLAs, they are then dead, yet they are not being removed.

Two ideas:

1. Run InnerSymbolDCE again in the populateLowFIRRTLToHW pipeline. The issue right now is that the BlackBoxReader pass is removing these annotations, these NLAs are then dead, but they are never removed.
2. Change the BlackBoxReader pass to prune these NLAs if it knows they are dead.

(2) is slightly annoying as finding uses requires global knowledge. It may be easier to just do (1). This will get better with #8704 as that will allow InnerSymbolDCE to run on the core dialects and then it will matter less if the FIRRTL pipeline leaves dead NLAs because they will be cleaned up in the core pipeline.

Thanks for your PR and looking to clean up the output.

[Tang-Haojin]

Thank you for your review! I have tried to add SymbolDCE (for hw.hierpath) and InnerSymbolDCE and found it worked. But it also slowed down the firtool for about 3%. I think it is not worth worthy to do such thing in the main branch and I will use it in my own repo. Thank you for your help again!