[FIRRTL] Question about different Annotation handling between SFC and firtool at Inline phase

[Nergy-TCGeneric]

Related PR: #2435

Sorry for bringing back old and dead SFC, but I'm rather curious about the reason 'diverged' behaviour between SFC and CIRCT at the inlining pass. I have a old custom SFC transform and want to migrate it to CIRCT. However, turns out CIRCT's ModuleInliner (ref) consumes Annotation attached to module when SFC preserved it by 'hoisting' it up to its parent.

Minimal example

Consider this simple Chisel code:

case class SampleTargetAnnotation(target: Target)
    extends SingleTargetAnnotation[Target] {
  override def duplicate(n: Target): Annotation = this.copy(target = n)
}

class AdderBundle extends Bundle {
  val in1 = Input(UInt(1.W))
  val in2 = Input(UInt(1.W))
  val out = Output(UInt(2.W))
}

class DUT extends RawModule with InlineInstance {
  val io = IO(new AdderBundle)
  io.out := io.in1 + io.in2
}

class DUTWrapper extends RawModule {
  val io = IO(new AdderBundle)

  val dut = Module(new DUT)
  dut.io.in1 := io.in1
  dut.io.in2 := io.in2
  io.out := dut.io.out

 // Before https://github.com/chipsalliance/chisel/pull/4717
  annotate {
    new ChiselAnnotation {
      def toFirrtl: Annotation = SampleTargetAnnotation(dut.toAbsoluteTarget)
    }
  }

  // After that PR
  annotate(this)(Seq(SampleTargetAnnotation(dut.toAbsoluteTarget)))
}

Both yield corresponding CHIRRTL output. Only picked up relevant annotations:

CHIRRTL

Chisel 3.6

circuit DUTWrapper :
  module DUT :
    output io : { flip in1 : UInt<1>, flip in2 : UInt<1>, out : UInt<2>}

    node _io_out_T = add(io.in1, io.in2)
    node _io_out_T_1 = tail(_io_out_T, 1)
    io.out <= _io_out_T_1

  module DUTWrapper :
    output io : { flip in1 : UInt<1>, flip in2 : UInt<1>, out : UInt<2>}

    io is invalid
    inst dut of DUT
    dut.io.in1 <= io.in1
    dut.io.in2 <= io.in2
    io.out <= dut.io.out

  {
    "class": "firrtl.passes.InlineAnnotation",
    "target": "DUTWrapper.DUT"
  },
  {
    "class": "reprod.SampleTargetAnnotation",
    "target": "~DUTWrapper|DUTWrapper/dut:DUT"
  },

Chisel 7

FIRRTL version 6.0.0
circuit DUTWrapper :%[[
  {
    "class":"firrtl.passes.InlineAnnotation",
    "target":"DUTWrapper.DUT"
  },
  {
    "class":"reprod.SampleTargetAnnotation",
    "target":"~DUTWrapper|DUTWrapper/dut:DUT"
  }
]]
  module DUT :
    output io : { flip in1 : UInt<1>, flip in2 : UInt<1>, out : UInt<2>}

    node _io_out_T = add(io.in1, io.in2)
    node _io_out_T_1 = tail(_io_out_T, 1)
    connect io.out, _io_out_T_1

  module DUTWrapper :
    output io : { flip in1 : UInt<1>, flip in2 : UInt<1>, out : UInt<2>}

    inst dut of DUT
    connect dut.io.in1, io.in1
    connect dut.io.in2, io.in2
    connect io.out, dut.io.out

And after corresponding inline pass, notice how that annotation's target gets changed:

Annotation target changes

SFC (firrtl.passes.InlineInstances)

target got hoisted, from ~DUTWrapper|DUTWrapper/dut:DUT to ~DUTWrapper|DUTWrapper.

  {
    "class":"reprod.SampleTargetAnnotation",
    "target":"~DUTWrapper|DUTWrapper/dut:DUT"
  }
======== Finished firrtl.passes.ResolveKinds ========
======== Starting firrtl.passes.InlineInstances ========
  {
    "class":"reprod.SampleTargetAnnotation",
    "target":"~DUTWrapper|DUTWrapper"
  },

MLIR (firrtl-inliner)

reprod.SampleTargetAnnotation itself is now gone, with NLA.

// -----// IR Dump Before Inliner (firrtl-inliner) //----- //
firrtl.circuit "DUTWrapper" {
  hw.hierpath private @nla [@DUTWrapper::@sym, @DUT]
  firrtl.module private @DUT(in %io_in1: !firrtl.uint<1>, in %io_in2: !firrtl.uint<1>, out %io_out: !firrtl.uint<2>) attributes {annotations = [{class = "firrtl.passes.InlineAnnotation"}, {class = "firrtl.transforms.NoDedupAnnotation"}, {circt.nonlocal = @nla, class = "reprod.SampleTargetAnnotation"}]} { ... }
firrtl.module @DUTWrapper(in %io_in1: !firrtl.uint<1>, in %io_in2: !firrtl.uint<1>, out %io_out: !firrtl.uint<2>) attributes {convention = #firrtl<convention scalarized>} { ... }

// -----// IR Dump After Inliner (firrtl-inliner) //----- //
firrtl.circuit "DUTWrapper" {
  firrtl.module @DUTWrapper(in %io_in1: !firrtl.uint<1>, in %io_in2: !firrtl.uint<1>, out %io_out: !firrtl.uint<2>) attributes {convention = #firrtl<convention scalarized>} { ... }

---

My guess is that,

1. IMDeadCodeElim can't remove empty module with Annotation attached, ends up with degraded verilog output quality:

// https://github.com/llvm/circt/blob/23ee54fe33849916be2753d9960677daa7b4d6e9/lib/Dialect/FIRRTL/Transforms/IMDeadCodeElim.cpp#L829
  if (!module.getAnnotations().empty()) {
    module.emitWarning() << "module `" << module.getName()
                         << "` is empty but cannot be removed "
                            "because the module has annotations "
                         << module.getAnnotations();
    return;
  }

2. Rather it was semantically nonsense to record 'inlined' target (though personally I'm the one who believes at least leaving 'this module was inlined into sth' can help downstream applications).

It seems that the first one is a strongest reason, but any other is deeply appreciated, if any.

[seldridge]

Thanks for the detailed issue report.

I went back through my notes on this and, as far as I can tell, I was trying to derive what I thought should happen as opposed to testing against what the SFC did here. (This is surprising to me, but this appears to be what the notes indicate.)

Reconstructing my original reasoning... if you have an annotation that is targeting a module and that module is inlined, then the only guaranteed thing that you can do is delete it. The only other option would be to error on this case and refuse to inline or flatten modules with annotations as this would force annotation deletion which is information destroying (though you can argue that module inlining is similarly information destroying).

The main problem I see with doing what the SFC did here is that it isn't always correct. Consider an annotation which is "Chisel Generator Class" that indicates what was the Scala class that generated that module. If the inline pass does what the SFC did here, then that annotation will be incorrect after inlining as the new annotation will point at a module it didn't generate. It might be legal to copy the annotation onto everything that was originally inside the inlined module, though. (You can construct an annotation with semantics that wants the exact inverse behavior, too.)

This whole thing highlights two different views of what annotations are. In the SFC, they had semantics which were governed by the def update(renames: RenameMap): Seq[Annotation]. Given a description of how names changes, the annotation could define what would happen. (Though I'm not sure how this could be used to get "inline" behavior unless the inline pass kind of lied and said that inlining actually "renamed" the inlined module to the parent?) In the MFC, annotations have no semantics other than those as interpreted by their pass. I.e., the pass is trying to always do the "information preserving" thing except when that is actually "delete this" which should also mean "delete the annotation".

though personally I'm the one who believes at least leaving 'this module was inlined into sth' can help downstream applications

Generally, we've tried to remove the need for this kind of thing with new features that avoid the need for knowing what the compiler did. E.g., probes and their ABI prevent the need for knowing what was inlined because the final path is provided as a macro.

[Nergy-TCGeneric]

I appreciate your such detailed explanation.

In the SFC, they had semantics which were governed by the def update(renames: RenameMap): Seq[Annotation]. Given a description of how names changes, the annotation could define what would happen.

Yeah. I'm also on the same page that these are all originated from the architecture itself. SFC manages every annotation at Circuit IR level, resembles 'centralized' form whereas MFC resembles somewhat 'distributed' form. One striking example IMO is a CombinationalPath.

  {
    "class":"firrtl.transforms.CombinationalPath",
    "sink":"~DUTWrapper|DUTWrapper>io_out",
    "sources":[
      "~DUTWrapper|DUTWrapper>io_in1",
      "~DUTWrapper|DUTWrapper>io_in2"
    ]
  }

// https://github.com/chipsalliance/firrtl/blob/64731bbb16142a2b09ccbe74ab41b76b7a265869/src/main/scala/firrtl/transforms/CheckCombLoops.scala#L83-L89
case class CombinationalPath(sink: ReferenceTarget, sources: Seq[ReferenceTarget]) extends Annotation {
  override def update(renames: RenameMap): Seq[Annotation] = {
    val newSources = sources.flatMap { s => renames(s) }.collect { case x: ReferenceTarget if x.isLocal => x }
    val newSinks = renames(sink).collect { case x: ReferenceTarget if x.isLocal => x }
    newSinks.map(snk => CombinationalPath(snk, newSources))
  }
}

This makes no sense in MFC because annotations (or technically, MLIR attributes) move along with IR and doesn't do anything by themselves. AFAIK SFC has no exact equivalent because FIRRTL was designed to be like that, so I'm pretty sure that SFC had to update these with update(RenameMap) API.

Historically, annotations grew out of three choices in the design of FIRRTL IR:

FIRRTL IR is not extensible with user-defined IR nodes.
FIRRTL IR is not parameterized.
FIRRTL IR does not support in-IR attributes.
(https://circt.llvm.org/docs/Dialects/FIRRTL/FIRRTLAnnotations/#motivation)

---

Quick question.
I wonder if FIRRTL has any plan to add any logical 'placeholder' IR in future.

Back then I was wondering if there's a direct way to express 'this diplomacy node belongs to this module'. Let's say if a diplomacy node is inside module named Bridge, in a FIRRTL path term this could be something like TOP|TOP/bridge:Bridge>OuterNode but OuterNode being... 'placeholder' for logical diplomacy node or such.

I know this is just a matter of interpretation, like this can be re-written like: "what module contains this node" and just use target as ordinary Annotations do without introducing any extra IR, but I believe this small question will not bite me ;)

[seldridge]

The general thought since the migration has been that we'd move away from annotations, except in very limited cases. Much of what they were doing (which was honestly pretty clever) was acting as an IR extension mechanism. Because they were so powerful (and the SFC infra couldn't support custom IR and passes were very inflexible in what they could support, e.g., low FIRRTL passes could not run on high FIRRTL and it would've been a lot of effort to make this work) their ease of use caused development stagnation on the FIRRTL spec side. MLIR helps here with both dialect mixing and ease of extension. Also, we have a pretty strong commitment to rapidly evolve FIRRTL to capture new concepts (probes, layers, assertions, properties, etc.) and rely on intrinsics for things we don't want to promote.

That example of the combinational path annotation is a good one. It has to do a lot of work to stay correct across things like type lowering which would be trivial to do if it was expressed in the IR. This does also hint that external module is deficient and should capture this directly with a body, or that combinational cycle detection shouldn't be done across external module boundaries and this is only done at link time (which may mean you do want an external module body to describe this for times where the external module is backed by a blackbox).

I wonder if FIRRTL has any plan to add any logical 'placeholder' IR in future.

I think you could do this with properties, though this is not a very well trodden path. Roughly, it lets you put metadata (strings, integers, etc. and aggregations of these via classes and objects) into hardware. You could approximate this with something like:

FIRRTL version 7.0.0
circuit Foo:
  public module Foo:
  output diplomaticNode: String
  
  propassign diplomaticNode = String("OuterNode")

This is obviously a very, very low-level IR which you'd want to wrap in a Chisel API to make it cleaner. However, it's intended to be flexible to capture whatever metadata needs users have. We're using this right now for a number of different efforts and it should be more usable soon.