Annotation Scattering and trivial processing Pass #1808
Conversation
There was a problem hiding this comment.
I really, really like the table-based approach here.
The if/else logic that branches on every op for the raw annotations configuration option bugs me, but this is just a temporary thing, right? (I'd prefer to get rid of this and shift to the new infra here as fast as we can.)
Miscellaneous items:
- There's a lot of static methods throughout that could use documentation. 😅
| if (auto extmod = dyn_cast<FExtModuleOp>(op)) | ||
| return extmod.getType().getInputs().size(); | ||
| if (auto mod = dyn_cast<FModuleOp>(op)) | ||
| return mod.getBodyBlock()->getArguments().size(); | ||
| return op->getNumResults(); |
There was a problem hiding this comment.
Can this avoid the dynamic casts and instead use a cast to FModuleLike? Something like:
cast<FModuleLike>(op).moduleType().getInputs()| ParseResult | ||
| FIRCircuitParser::importAnnotationsRaw(SMLoc loc, StringRef circuitTarget, | ||
| StringRef annotationsStr, | ||
| SmallVector<Attribute> &attrs) { | ||
|
|
||
| auto annotations = json::parse(annotationsStr); | ||
| if (auto err = annotations.takeError()) { | ||
| handleAllErrors(std::move(err), [&](const json::ParseError &a) { | ||
| auto diag = emitError(loc, "Failed to parse JSON Annotations"); | ||
| diag.attachNote() << a.message(); | ||
| }); | ||
| return failure(); | ||
| } | ||
|
|
||
| json::Path::Root root; | ||
| llvm::StringMap<ArrayAttr> thisAnnotationMap; | ||
| if (!fromJSONRaw(annotations.get(), circuitTarget, attrs, root, | ||
| getContext())) { | ||
| auto diag = emitError(loc, "Invalid/unsupported annotation format"); | ||
| std::string jsonErrorMessage = | ||
| "See inline comments for problem area in JSON:\n"; | ||
| llvm::raw_string_ostream s(jsonErrorMessage); | ||
| root.printErrorContext(annotations.get(), s); | ||
| diag.attachNote() << jsonErrorMessage; | ||
| return failure(); | ||
| } | ||
|
|
||
| return success(); | ||
| } |
There was a problem hiding this comment.
This is introducing some duplicated code with importAnnotations. Can these be combined? I also see that importAnnotations is intended to be short-lived once we port everything to use the pass scattering approach.
There was a problem hiding this comment.
The code should be temporary and should cease existing once the transition happens.
| struct tokenAnnoTarget { | ||
| StringRef circuit; | ||
| SmallVector<std::pair<StringRef, StringRef>> instances; | ||
| StringRef module; | ||
| StringRef name; | ||
| SmallVector<fieldOrIndex> agg; | ||
| }; |
There was a problem hiding this comment.
Extremely nitty: I would change this to match the SFC names and order for these fields:
struct ReferenceTarget {
StringRef circuit;
StringRef module;
SmallVector<std::pair<StringRef, StringRef>> path;
StringRef ref;
SmallVector<fieldOrIndex> component;
}At minimum, just changing the order would be good. (I have found that "path", "ref", and "component" to be confusing.)
There was a problem hiding this comment.
The module comes logically after the path and is the thing the ref is in, regardless of what is in path.
| LogicalResult applyAnnotation(DictionaryAttr anno, applyState state); | ||
|
|
||
| bool ignoreUnhandledAnno = false; | ||
| bool ignoreClasslessAnno = false; |
There was a problem hiding this comment.
I like this classless infra. (I think this is new?) I like writing classless annotations for testing, but these should not exist coming from Chisel. Having a mechanism to enable/disable this behavior makes sense to me. 💯
| @@ -1,5 +1,4 @@ | |||
| // RUN: circt-opt --pass-pipeline='firrtl.circuit(firrtl-lower-annotations)' -split-input-file %s |FileCheck %s | |||
| // XFAIL: * | |||
test/Dialect/FIRRTL/annotations.mlir
Outdated
| firrtl.circuit "FooNL" attributes {annotations = [ | ||
| {class = "circt.test", nl = "nl", target = "~FooNL|FooNL/baz:BazNL/bar:BarNL>w"}, | ||
| {class = "circt.test", nl = "nl2", target = "~FooNL|FooNL/baz:BazNL/bar:BarNL>w2.b[2]"} | ||
| ]} { |
There was a problem hiding this comment.
Can you update this test to hit the local annotation path, too?
Or: what do you want to do about fully testing this? Land it and then start migrating existing annotations.fir tests? I am fine with this approach, too.
There was a problem hiding this comment.
I want to migrate existing tests.
Yes, it's a bit of a flag-day kind of conversion. The if stuff is just so it can coexist for a while. |
Add a pass to handle all annotation scattering. This pass is table driven, with customizable scattering per-annotation-class. When this is fleshed out, it will replace the annotation handling code in the parser.
Right now, this supports a couple testing annotation to make test cases against.
Until this is live in the pipelines, add an option to the parser to bypass annotation handling and scattering to enable testing.