[OM] Rework ClassOp to use fields terminator

[leonardt]

This changes the OM dialect Class/ExternClass to use a terminator operation similar to hw.output.

The ClassFields and ClassExternFields operations use a custom printer/parser to provide a syntax similar to hw.module where fields use the format @name %val : !type for non-extern fields and @name : !type for extern fields.

Internally, the fields operations use attributes to store required information. Both fields operations store a list of fields to preserve definition order. Non-extern fields store a mapping from field name to operand index. Extern fields store a mapping from field name to type.

The fields operations provide an API for getting the type of a field based on the name. The extern fields looks this up in the attribute dictionary, while the non-extern fields operation looks up the operand index and retrieves the type from the appropriate operand. This ensures that non-extern fields map to the latest type in cases where the operand type changes after construction.

The Class/ExternClass operations provide a convenience API that lifts the fields API to avoid having to frequently request the underlying fields operation.

The firrtl LowerClasses logic requires changes to use this API when constructing OM classes, as well as in the dialect conversion logic to handle the new operations.

The OM evaluator similarly is updated for the new fields API.

The OM linker is updated to skip constructing the field name to type map and instead using the getFieldType API provided by the classOp.

The OM object field verifier also no longer needs to construct the field name to type maps and instead can just use the new API.

Quite a few tests required updating for the new field syntax.

Two TODOs left:

• Is there a way to attach a note to a parser error? The old code used this when encountering duplicate fields (the note pointed to the previous definition). Because this check is now done at parse time, we are emitting parser errors instead of an InFlightDiagnostic.
• Right now the getFieldType API uses a std::optional style for handling cases when the type of a field doesn't exist (i.e. the provided name is not field). Another option would be to expose a hasField API.

[leonardt]

Ref #7078

[uenoku]

I think this is great. Thank you for working on this! It's great improvement to use terminator instead of individual field class op. I'm wondering if we can do further: can we encode field names into its class declaration? e.g.

om.class @NestedField1() -> (baz: i1) {
  %0 = om.constant 1 : i1
  om.class.fields %0 : i1
}

om.class @NestedField2() -> (bar: !om.class.type<@NestedField1>) {
  %0 = om.object @NestedField1() : () -> !om.class.type<@NestedField1>
  om.class.fields %0 : !om.class.type<@NestedField1>
}

With that change I think we don't need ClassExternFieldsOp since extern class declaration has the same information (and interface OMClassFieldsLike would be unnecessary as well).

[leonardt]

Thanks for the comments @uenoku , I'll work on them and let you know when it's ready for another review.

Moving fields to the class was something @mikeurbach mentioned at the outset. I think we thought that this approach seemed like a simpler incremental change, but now I agree that using the class would remove a layer of indirection (going from class op to fields op) and simplify things. Will look into that.

I don't know have experience with the issues with symbols so I'd trust your opinion on that, so happy to move to "raw" field names.

[mikeurbach]

Sorry for the delay in giving a serious review. I can at least respond to a couple of the points you guys discussed...

I like the idea of putting the field names and types on the class... that is conceptually even closer to the design point that HW dialect settled on, and if it simplifies things, it might be worth going straight to that in this change. At this point @leonardt has iterated on different designs than I have, so I think it's up to you if it's worth doing more iterations or stopping at this point. Personally, I think we're not in a rush to get this out, so if it is worth looking into I vote go for it.

Regarding symbols... this was one of my original sins. Even after we made om.class.field no longer a Symbol op, i let them print and parse as symbol names, which is just plain misleading when you look at the textual IR. Regardless of how they're printed or parsed, symbol names are internally StringAttr, but using the @foo syntax is the printed form should probably be reserved for actual symbol names (the sym_name attribute of an op that declares the Symbol trait).

I'll take a pass through the PR for any low-level coding comments, which might be applicable regardless of whether we stay at this design point or move towards Hideto's suggestion.

[mikeurbach]

Just some passing comments in addition to what Hideto left. Overall, I think this looks great for the current design point--the changes in the various passes look nice and straightforward, nice work!

[leonardt]

Updated the syntax to use a return type style, added two review comments on code areas that we might be able to improve further

[leonardt]

Will need to rebase on main to make sure there's no merge issues

[mikeurbach]

I took a pass through the latest revision and it's looking really good. I will go through and add a more detailed review. @uenoku please take a look as well.

[mikeurbach]

I took another look through this. No major comments. Let's see how it looks on some large designs but after that I think it should be good to merge.

[uenoku]

Oh, sorry. I have missed the review request. Thank you for addressing comments! The new representation does look good to me!

[leonardt]

CC @mikeurbach @uenoku if I might get a final review on this change that was added after the initial approvals: 85e6d6a

Basically: FusedLoc will "merge" identical locations (so they are stored as a set). This is problematic when we're trying to lookup per-field locations by using the fused location of the fields op. This changes it so that the metadata field of FusedLoc is used to store the original location array so that locations are not merged and per field locations can be looked up by index. I added some assertions in hopes that it might catch cases when this API/invariant is not obeyed (e.g. by someone constructing a fields op directly and adding a fused loc without the metadata attribute). There likely might be a better way to enforce this invariant, or at least some more assertions we could include to catch cases when this invariant is broken.

[mikeurbach]

Yep, I looked through that and it makes sense to me. I think the assertions are good--that's what they're for: https://llvm.org/docs/CodingStandards.html#assert-liberally. If user input could lead to the invariant being violated, that's an error, but if only compiler authors could lead to the invariant being violated, that's a prime candidate for assertions.