Page MenuHomePhabricator

Introduce MLIR Op Properties
DraftPublic

Authored by mehdi_amini on Jan 13 2023, 5:39 PM.
This is a draft revision that has not yet been submitted for review.

Details

Summary

This new features enabled to dedicate custom storage inline within operations.
This storage can be used as an alternative to attributes to store data that is
specific to an operation. Attribute can also be stored inside the properties
storage if desired, but any kind of data can be present as well. This offers
a way to store and mutate data without uniquing in the Context like Attribute.
See the OpPropertiesTest.cpp for an example where a struct with a
std::vector<> is attached to an operation and mutated in-place:

struct TestProperties {

int a = -1;
float b = -1.;
std::vector<int64_t> array = {-33};

};

More complex scheme (including reference-counting) are also possible.

The only constraint to enable storing a C++ object as "properties" on an
operation is to implement three functions:

  • convert from the candidate object to an Attribute
  • convert from the Attribute to the candidate object
  • hash the object

Optional the parsing and printing can also be customized with 2 extra
functions.

Diff Detail

Event Timeline

mehdi_amini created this revision.Jan 13 2023, 5:39 PM

Nice. This is an exciting evolution of the operation model in MLIR. Attributes are really slow for many trivial things like storing an integer.

mlir/include/mlir/IR/OpBase.td
182

that's a lot of newlines

229

should this field be near the top?

mlir/include/mlir/IR/OpDefinition.h
1612

can you document these a little?

1742

docs please

mlir/include/mlir/IR/Operation.h
233

nice cider autoformatting

751

Can you use a fixed-width type for the properties object size field? I imagine int32_t would be sufficient

mlir/include/mlir/IR/OperationSupport.h
433
473–522
1114

Maybe we should add an ODSSupport.h file? DialectImplementation.h has the FieldParser stuff used by ODS too

1116

Please drop all the leading ::mlir::'s

mlir/lib/AsmParser/Parser.cpp
1964

does std::move even do anything here?

mlir/test/IR/properties.mlir
21

newline pls

mlir/tools/mlir-tblgen/FormatGen.cpp
180

keyword and name of the token don't line up?

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
1116

these should get all the ::llvm::/::mlir:: prefixes

minor cleanups

mehdi_amini marked 12 inline comments as done.

Address Jeff's comments (easy ones)

mlir/include/mlir/IR/Operation.h
751

Reminds me that I'm concerned about the alignment for the Properties pointer here, need to think more about this...
Leaving this comment open for now.

Move propertiesStorageSize to Operation member, stealing 8 bits from numRegions. This limits the maximum size of Properties to 2048B (anything that large does not seem reasonnable anyway!)

mehdi_amini marked an inline comment as done.Jan 14 2023, 11:32 AM