[FIRRTL] Annotations Targeting Aggregates of Probes Not Found

[seldridge]

trafficstars

Annotation resolution of aggregates of probes should produce a better error message. Currently, this fails to find the "name". The actual issue is that it is an annotation targeting a probe.

Consider:

FIRRTL version 3.3.0
circuit Foo: %[[
  {"class":"firrtl.transforms.DontTouchAnnotation","target":"~Foo|Foo>a"},
  {"class":"firrtl.transforms.DontTouchAnnotation","target":"~Foo|Foo>b"}
]]
  module Foo:
    output a: Probe<UInt<1>>[1]
    output b: {a: Probe<UInt<1>>}

This produces:

firrtl-snippets/ProbeAnnotations.fir:2:1: error: cannot find name 'a' in Foo
circuit Foo: %[[
^
firrtl-snippets/ProbeAnnotations.fir:2:1: error: Unable to resolve target of annotation: {class = "firrtl.transforms.DontTouchAnnotation", target = "~Foo|Foo>a"}
circuit Foo: %[[
^
firrtl-snippets/ProbeAnnotations.fir:2:1: error: cannot find name 'b' in Foo
circuit Foo: %[[
^
firrtl-snippets/ProbeAnnotations.fir:2:1: error: Unable to resolve target of annotation: {class = "firrtl.transforms.DontTouchAnnotation", target = "~Foo|Foo>b"}
circuit Foo: %[[
^

[dtzSiFive]

Previously: 819a037adf3d3826e9a038095414e2438932f0fe 2694312615ffe7117b6d97392c0aa864f06c1b1a

Agreed, the user experience here is poor.

Same will occur for annotations that target properties (or aggregate with only properties).

[dtzSiFive]

LOA's behavior for empty bundles-- here, bundles with no non-hw elements-- may be good to revisit, it elides them if they have no "HW" elements but maybe they should be left around (so Foo>b is a valid annotation target, for example?).

The title suggests this was the original surprising behavior ("Aggregates of Probes"), which agreed is .. unexpected. Putting a HW element in there, even zero-width, would change this behavior re:ability to annotate Foo>b!

I'll take a look at this.

[dtzSiFive]

Providing better diagnostics for attempts to annotate non-HW things would be nicer, for sure (Foo>a or Foo>b.a).

LowerAnnotations running before LOA would help with this, previously one pain was LOA not supporting inner symbols (which it does now-- inner symbol goes to the HW-only portion, cannot be on non-HW only) and how it might work if an annotation is on an open aggregate, ideally without annotation-specific logic (a bit like LowerTypes).

This also means at least temporarily an annotation would be on something that isn't a FIRRTLBaseType which may be okay but IMHO should be avoided until we're very sure we want that and define what is supposed and what exactly that means and how to "lower" an aggregate that has some arbitrary annotation.

Refactoring our various aggregate-type-lowering (cc LowerSignatures?) might help as motivation to factor our annotation "lowering", as these are some common problems once we address the semantic ones. Haven't thought a lot about this, glad to have more eyes on these details.

The current approach is prioritizing correctness and simplicity at cost of the diagnostics provided, for better or for worse.

Thanks for filing!

[mikeurbach]

I was searching for open issues and found this one. Just to add, I guess the issue isn't just Probes, but any non-FIRRTLBaseType, including properties. I found this by writing some module that has a bundle including properties on its interface, and don't touching each port on the interface:

FIRRTL version 3.3.0

circuit Example : %[[
  {
    "class": "firrtl.transforms.DontTouchAnnotation",
    "target": "~Example|Example>myport.myint"
  }
]]
  module Example :
    input myport : { myint : Integer }

myport is lowered by LowerOpenAggs, and then the annotation is pointing to something that doesn't exist:

example.fir:3:1: error: cannot find name 'myport' in Example                                                                                                                                                                                                                    
circuit Example : %[[                                                                                                                                                                                                                                                           
^                                                                                                                                                                                                                                                                               
example.fir:3:1: error: Unable to resolve target of annotation: {class = "firrtl.transforms.DontTouchAnnotation", target = "~Example|Example>myport.myint"}                                                                                                                     
circuit Example : %[[                                                                                                                                                                                                                                                           
^                                                                                                                                                                                                                                                                               
// -----// IR Dump After LowerFIRRTLAnnotations Failed (firrtl-lower-annotations) //----- //                                                                                                                                                                                    
firrtl.circuit "Example" {                                                                                                                                                                                                                                                      
  firrtl.module @Example(in %myport_myint: !firrtl.integer) attributes {convention = #firrtl<convention scalarized>} {                                                                                                                                                          
  }                                                                                                                                                                                                                                                                             
}

[dtzSiFive]

Yep, bulk-using dontTouch is most common cause of pain here. Using "public" instead of an adhoc encoding of it via dontTouch would help.

And yep all non-hw things cannot be annotated presently, see my comment above. As you say they're simply not there for LowerAnnotations.