llvm / circt

Circuit IR Compilers and Tools
https://circt.org
Other
1.67k stars 296 forks source link

[FIRRTL] Resolve DontTouchAnnotation earlier in the pipeline #2102

Closed fabianschuiki closed 2 years ago

fabianschuiki commented 2 years ago

Now that #2081 has landed, FIRRTL declarations (wire, node, etc.) can have their own symbol through the inner_sym attribute. This now opens up the possibility to translate the DontTouchAnnotation to a symbol name much earlier in the pipeline. Currently the translation happens right in LowerToHW. With the inner_sym, the FIRParser could already add a symbol name to the op when it is first created, and all the DontTouchAnnotation processing could then be dropped from LowerToHW.

This requires some extra care since quite a few passes might add their own DontTouchAnnotations, which would have to be changed so they attach a symbol to the ops instead.

seldridge commented 2 years ago

This makes a lot of sense to me. 👍

seldridge commented 2 years ago

One thing to consider, DontTouchAnnotation currently means "do not remove and do not optimize through". Does an inner symbol name provide both of these guarantees or just the former?

fabianschuiki commented 2 years ago

Yeah good point. I've started to update our uses of AnnotationSet(...).hasDontTouch() to also consider the presence of inner_sym as an indicator for don't-touchy-ness. If we entirely map DontTouchAnnotation to the presence of an inner_sym, then that might actually become easier.

I do have some anxiety about really changing this, just because we might at some point decide that we want to keep tabs on any of these declarations without preventing optimization, such that the presence of a symbol doesn't necessarily indicate a don't touch anymore. But that might also be something that we want to deal with when we come across it.

seldridge commented 2 years ago

DontTouchAnnotation is, on main, resolved as a DontTouchAnnotation during parsing or in LowerAnnotations to a symbol except if it targets a field of an aggregate. This is then converted to a symbol during LowerTypes.