[HW] Disallow duplicate field names in HW aggregate types

[fzi-hielscher]

hw.struct_extract, hw.struct_inject and hw.union_extract refer to their respective field by name. However, field names of structs and union types are currently not enforced to be unique. This can cause ambiguities, e.g.:

hw.module @extract(in %a: i8, in %b: i8, out o: i8) {
    %0 = hw.struct_create (%a, %b) : !hw.struct<foo: i8, foo: i8>
    %1 = hw.struct_extract %0["foo"] :  !hw.struct<foo: i8, foo: i8>
    hw.output %1 : i8
}

> circt-opt ... --canonicalize

hw.module @extract(in %a : i8, in %b : i8, out o : i8) {
    hw.output %a : i8
}

This PR prevents the creation of HW aggregate types with duplicate field names.

As @seldridge pointed out to me, the FIRRTL dialect refers to subfields by index rather than by name. I've prepared a follow-up commit adopting this for StructExtract and StructInject ops. Allowing ambiguous names in structs is probably not a good idea either way. But it should be marginally more efficient, as it avoids iterating over the field names to find the matching one.

[seldridge]

Approach looks good! Some thoughts in the review.

[seldridge]

Checking this during parsing and verification results means the check has to be duplicated. Is the error just as good if you ignore the parsing check and wait for the verifier to blow up?

[fzi-hielscher]

I originally just did the verify method. It does the trick, but a failure there will dump a full stack trace, which I found too verbose and somewhat intimidating. 😄

[seldridge]

Fair. One thing you can do to further improve this is to use a better location for the error. Currently the error looks like:

# circt-opt Foo.mlir 
Foo.mlir:2:65: error: duplicate field name 'foo'
    %0 = hw.struct_create (%a, %b) : !hw.struct<foo: i8, foo: i8, foo: i8>
                                                                ^
Foo.mlir:2:74: error: duplicate field name 'foo'
    %0 = hw.struct_create (%a, %b) : !hw.struct<foo: i8, foo: i8, foo: i8>
                                                                         ^

If you use:

diff --git a/lib/Dialect/HW/HWTypes.cpp b/lib/Dialect/HW/HWTypes.cpp
index e89d9b3ab..0c22baef2 100644
--- a/lib/Dialect/HW/HWTypes.cpp
+++ b/lib/Dialect/HW/HWTypes.cpp
@@ -248,13 +248,14 @@ static ParseResult parseFields(AsmParser &p,
       mlir::AsmParser::Delimiter::LessGreater, [&]() -> ParseResult {
         std::string name;
         Type type;
+
+        auto fieldLoc = p.getCurrentLocation();
         if (p.parseKeywordOrString(&name) || p.parseColon() ||
             p.parseType(type))
           return failure();
 
         if (!nameSet.insert(name).second) {
-          p.emitError(p.getCurrentLocation(),
-                      "duplicate field name \'" + name + "\'");
+          p.emitError(fieldLoc, "duplicate field name \'" + name + "\'");
           // Continue parsing to print all duplicates, but make sure to error
           // eventually
           hasDuplicateName = true;

You get:

# circt-opt Foo.mlir 
Foo.mlir:2:58: error: duplicate field name 'foo'
    %0 = hw.struct_create (%a, %b) : !hw.struct<foo: i8, foo: i8, foo: i8>
                                                         ^
Foo.mlir:2:67: error: duplicate field name 'foo'
    %0 = hw.struct_create (%a, %b) : !hw.struct<foo: i8, foo: i8, foo: i8>
                                                                  ^

[seldridge]

LGTM

Nice improvement, thanks!

[fzi-hielscher]

Thanks a lot for the review. I'll get the commit changing the StructExtract/Inject operations PR ready next week, assuming there is no specific reason to keep the reference by-name.