Page MenuHomePhabricator

[mlir] Replace usages of Identifier with StringAttr
ClosedPublic

Authored by rriddle on Nov 9 2021, 6:43 PM.

Details

Summary

Identifier and StringAttr essentially serve the same purpose,
i.e. to hold a string value. Keeping these seemingly identical
pieces of functionality separate has caused problems in certain situations:

  • Identifier has nice accessors that StringAttr doesn't
  • Identifier can't be used as an Attribute, meaning strings are often duplicated between Identifier/StringAttr (e.g. in PDL)

The only thing that Identifier has that StringAttr doesn't is support
for caching a dialect that is referenced by the string (e.g. dialect.foo).
This functionality is added to StringAttr, as this is useful for StringAttr
in generally the same ways it was useful for Identifier.

Diff Detail

Event Timeline

rriddle created this revision.Nov 9 2021, 6:43 PM
rriddle requested review of this revision.Nov 9 2021, 6:43 PM
lattner accepted this revision.Nov 9 2021, 9:15 PM

Awesome, this is a great direction that will simplify things. Thank you!

mlir/include/mlir/IR/BuiltinAttributes.h
926

Randomly, this alone will be really handy. I have built a lot of densemaps with "attribute" as the key instead of StringAttr just to work around this :-)

mlir/include/mlir/IR/BuiltinAttributes.td
935

Should this be size_t instead of unsigned?

This revision is now accepted and ready to land.Nov 9 2021, 9:15 PM

LG, thanks River!

bondhugula added inline comments.
mlir/include/mlir/IR/BuiltinAttributes.td
918

is a -> is

919

Add a trailing "null otherwise"?

This revision was automatically updated to reflect the committed changes.
rriddle marked 4 inline comments as done.

Thanks for the review!

mlir/include/mlir/IR/BuiltinAttributes.td
935

Thanks for catching this, copy over from the old Identifier class.