This class is looking up a dialect prefix on the identifier on initialization
and keeping a pointer to the Dialect when found.
The NamedAttribute key is now a DialectIdentifier.
Paths
| Differential D95418
Introduce a new DialectIdentifier structure, extending Identifier with a Dialect information ClosedPublic Authored by mehdi_amini on Jan 25 2021, 10:08 PM.
Details Summary This class is looking up a dialect prefix on the identifier on initialization The NamedAttribute key is now a DialectIdentifier.
Diff Detail
Event TimelineHerald added subscribers: teijeong, rdzhabarov, tatianashp and 13 others. · View Herald TranscriptJan 25 2021, 10:08 PM Comment Actions The name DialectIdentifier strikes me as weird on first glance, but I think it's slowly being normalized the more I look at it. I like the idea of making it more efficient to access a context/dialect from a dialect prefixed identifier. Do you have any performance impact numbers for the change to NamedAttribute? Comment Actions Isn't it still just an Identifier? The dialect was always there, it was just previously encoded in the string and is now separate Comment Actions
Are you suggesting to not have two classes and just merge the two classes here? Comment Actions
I think that route is going to end up being better, given that it reduces the number of lookups to 1 in the common case. If not that, then diverging from Identifier and still uniquing. Before suggesting to just merge them, I was trying to see what else Identifier is used for aside from Attribute names, but I couldn't find anything. Comment Actions Some Locations may be using Identifier like in FileLineColLocationStorage. Comment Actions
If we uniqued DialectIdentifier(the Identifier part + the Dialect */MLIRContext *), then OperationName wouldn't grow AFAICS. It would improve memory(no extra pointer per instance of a specific identifier) and reduce lookup time. I don't think we should replace Identifier directly given the other usages, but it still seems interesting to optimize the important cases that do rely on this type of identifier. Having a proper representation for an identifier that can be dialect prefixed seems useful. Comment Actions Oh I see what you mean, we'd replace the current llvm::StringSet<llvm::BumpPtrAllocator &> identifiers; in the MLIRContext with something like llvm::StringMap<Dialect*, llvm::BumpPtrAllocator &> identifiers;? Something I need to figure out is what to do on dialect loading in the Context, I propose on such event to loop over the identifiers and if any is prefixed with the loaded dialect then set the Dialect pointer, WDYT? mehdi_amini retitled this revision from Introduce a new DialectIdentifier structure, extending Identifier with a Dialect information to Extend the Identifier storage with a pointer to the Dialect (or the MLIRContext).Jan 27 2021, 7:35 PM mehdi_amini retitled this revision from Extend the Identifier storage with a pointer to the Dialect (or the MLIRContext) to Introduce a new DialectIdentifier structure, extending Identifier with a Dialect information. Comment ActionsUpdate to fold the pointer into the Identifier storage. This revision is now accepted and ready to land.Jan 28 2021, 9:40 AM Comment Actions Nice. Really clean now! LGTM after adding examples.
mehdi_amini marked an inline comment as done. Comment Actionsbeef up the main description of the Identifier
This revision was landed with ongoing or failed builds.Jan 28 2021, 4:05 PM Closed by commit rGe9dc94291e7d: Introduce a new DialectIdentifier structure, extending Identifier with a… (authored by mehdi_amini). · Explain Why This revision was automatically updated to reflect the committed changes. Comment Actions Seems you retitled the commit, but that got reverted to original description again on push.
Revision Contents
Diff 319207 mlir/include/mlir/Dialect/Affine/IR/AffineMemoryOpInterfaces.td
mlir/include/mlir/Dialect/Affine/IR/AffineOps.h
mlir/include/mlir/Dialect/Affine/IR/AffineOps.td
mlir/include/mlir/IR/Attributes.h
mlir/include/mlir/IR/Builders.h
mlir/include/mlir/IR/BuiltinAttributes.h
mlir/include/mlir/IR/Identifier.h
mlir/include/mlir/IR/OperationSupport.h
mlir/lib/CAPI/IR/BuiltinAttributes.cpp
mlir/lib/IR/Builders.cpp
mlir/lib/IR/BuiltinDialect.cpp
mlir/lib/IR/MLIRContext.cpp
mlir/lib/IR/OperationSupport.cpp
mlir/lib/Parser/AttributeParser.cpp
mlir/test/lib/Dialect/Test/TestDialect.cpp
mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
mlir/tools/mlir-tblgen/RewriterGen.cpp
mlir/tools/mlir-tblgen/StructsGen.cpp
mlir/unittests/TableGen/StructsGenTest.cpp
|