Page MenuHomePhabricator

[mlir][ODS] Support specialized Attribute class for Enums
ClosedPublic

Authored by vinograd47 on Mar 3 2021, 12:09 AM.

Details

Summary

Add a feature to EnumAttr definition to generate
specialized Attribute class for the particular enumeration.

This class will inherit StringAttr or IntegerAttr and
will override classof and getValue methods.

With this class the enumeration predicate can be checked with simple
RTTI calls (isa, dyn_cast) and it will return the typed enumeration
directly instead of raw string/integer.

Based on the following discussion:
https://llvm.discourse.group/t/rfc-add-enum-attribute-decorator-class/2252

Diff Detail

Event Timeline

vinograd47 created this revision.Mar 3 2021, 12:09 AM
vinograd47 requested review of this revision.Mar 3 2021, 12:09 AM
vinograd47 updated this revision to Diff 327758.Mar 3 2021, 5:46 AM

Fixed clang-tidy issues.

rriddle added inline comments.Mar 5 2021, 7:58 AM
mlir/include/mlir/IR/OpBase.td
1195

In what situations is this not desirable? This seems always desirable given that we could add utility methods for things like symbolization, getting the case value, etc.

1230

Why?

mlir/tools/mlir-tblgen/EnumsGen.cpp
324

You shouldn't need this check. classof should be guaranteed to never be called on a null value.

330

nit: Spell out auto here and above.

353

Drop trivial braces here.

vinograd47 added inline comments.Mar 5 2021, 8:57 AM
mlir/include/mlir/IR/OpBase.td
1195

In what situations is this not desirable? This seems always desirable given that we could add utility methods for things like symbolization, getting the case value, etc.

I've disabled this feature for StrEnumAttr be default, because it might be less effective. The Enum getValue() method in specialized class will call symbolizeEnum each time (doing the string switch). So, in general StringRef getValue() and string comparison might be efficient than this approach. genSpecializedAttr allows to choose between type-safety and efficiency. In my local fork I did some optimization to overcome this - added separate mutable field to StringAttr to hold actual enumeration value (as uint64_t). If the enum attribute is creates with specialized attribute get/getChecked method, then this field is set. But this approach is fragile. For example, we might have 2 dialects, which has separate enums with the same string values, but different indexes (eg. "Foo::CaseA == 1" and Bar::Case1 == 5), but both of them will be represented as single "Case1" StringAttr.

vinograd47 updated this revision to Diff 329255.Mar 9 2021, 2:07 AM

Rebased and fixed review comments.

vinograd47 marked 3 inline comments as done.Mar 9 2021, 2:08 AM

@rriddle I've fixed your remarks in EnumsGen.cpp.

flaub added a subscriber: flaub.Mar 10 2021, 12:17 PM
rriddle accepted this revision.Tue, Mar 16, 11:40 AM
This revision is now accepted and ready to land.Tue, Mar 16, 11:40 AM