This is an archive of the discontinued LLVM Phabricator instance.

Introduce MLIR Op Properties
ClosedPublic

Authored by mehdi_amini on Jan 13 2023, 5:39 PM.

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.

A new options is introduced to ODS to allow dialects to specify:

let usePropertiesForAttributes = 1;

When set to true, the inherent attributes for all the ops in this dialect
will be using properties instead of being stored alongside discardable
attributes.
The TestDialect showcases this feature.

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
1643

can you document these a little?

1758

docs please

mlir/include/mlir/IR/Operation.h
238

nice cider autoformatting

838

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
503
539–638
1257

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

1259

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

mlir/lib/AsmParser/Parser.cpp
1992

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
1162

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
838

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
mehdi_amini edited the summary of this revision. (Show Details)

Add a new ODS dialect flag "usePropertiesForAttributes" which when set to true makes all inherent attributes stored as properties for this dialect. The test dialect is updated to showcase this.

The idea is to make this the default in the future and deprecate storing inherent attributes alongside discardable ones.

rebase and some small fixes

ubfx added a subscriber: ubfx.Apr 11 2023, 5:35 AM

Rebase and minor updates to make initialization more robust

mehdi_amini published this revision for review.Apr 13 2023, 9:59 PM
jpienaar added inline comments.
mlir/lib/Dialect/Shape/IR/Shape.cpp
1143

Mmmm, if we always used OpAdaptor, then these could all have been hidden below that here right?

Oh (and I haven't look at rest) is OpAdaptor also expanded?

Started going through, made it about half way, will need another pass or two.

mlir/include/mlir/IR/OpBase.td
197

Tablegen constructs have generally used $_blah for these. Why not do that for properties?

199–200

Can you add a Format reference like the other fields?

1159

N -> n

1164

Extra newline here

2475–2479

When will this be used? What benefit is there to making this open?

mlir/include/mlir/IR/OpDefinition.h
1730

// -> ///

Same below

1733
1741
1818–1820

Can you document this? Also, why not wrap this in a constexpr function?

1919–1931

Not putting this in the else branch?

1979

Did you mean customPrintProperties here?

mlir/include/mlir/IR/Operation.h
97

Can you define a strongly typed wrapper class for this void *? The use of void * makes me a tad uncomfortable here.

435

Why std::optional? That doesn't match any other API usage we have for this type of method, I'd rather not diverge.

437–439

Can you document these methods?

495–496
503–504

Same throughout

838

Did this get resolved?

841–846

Let's please use strongly typed API here instead of void *.

996

// -> ///

mlir/include/mlir/IR/OperationSupport.h
124
552

Why cast just to resolve back to op? Also it'd be nice to add context as to why it goes to getDiscardableAttr in this case, which realistically it could just be getAttr until those methods get remove.

rriddle added inline comments.Apr 16 2023, 11:33 PM
mlir/include/mlir/IR/OperationSupport.h
583
585
955–957

Please document this method.

1256–1257

Let's resolve this. I'd be fine with a support file as Jeff suggested.

mlir/include/mlir/TableGen/Operator.h
355

Please document this API.

mehdi_amini marked 4 inline comments as done.Apr 17 2023, 6:26 AM
mehdi_amini added inline comments.
mlir/include/mlir/IR/OpBase.td
2475–2479

What I had in mind when I added this was the ability to use the same class for multiple ops in the same dialect. It does not seem to carry its weight right now though, removing.

mlir/lib/Dialect/Shape/IR/Shape.cpp
1143

Yes the adaptor constructor has been updated, see here for example: https://reviews.llvm.org/D141742#change-ewmZYaiO8SOS

address a very small part of the comments :)

jpienaar added inline comments.Apr 18 2023, 1:36 PM
mlir/lib/Dialect/Shape/IR/Shape.cpp
1143

What is stopping us from switching to those? :) (e.g., if we have folks going through all the interfaces, perhaps we can fold it in). I know that couples 2 changes, I could try and "quickly" do that if you think that makes sense. Then your change here becomes smaller. WDYT? (I could also delay till later/after next 2 conferences :))

mehdi_amini marked 23 inline comments as done.

Address comments

mlir/include/mlir/IR/Operation.h
97

Ah! Indeed I hated the void* and wrongly punted on this...

435

We need to differentiate whether the attribute is set from whether there is no inherent attribute with this name.
That is: returning std::nullopt is signaling a different case than returning an Attribute{} ; and both cases can happen here.

838

Yes: there is now a field (propertiesStorageSize) on the operation that stores the size, and this is solving my alignment issue as well.

mlir/include/mlir/IR/OperationSupport.h
552

Why cast just to resolve back to op?

It's a trick: it delays resolving the call to later when the Operation class is available. Without it I get:error: member access into incomplete type 'mlir::Operation'

Also it'd be nice to add context as to why it goes to getDiscardableAttr in this case, which realistically it could just be getAttr until those methods get remove.

Right it could be getAttr(), but seemed more logical to me to resolve the call to where it'll end up: if someone traces the code I'm saving them some mental load in carrying invariants through the call stack.

mlir/include/mlir/TableGen/Operator.h
355

Removed API

mlir/lib/Dialect/Shape/IR/Shape.cpp
1143

We should look into a default implementation building the adaptor and forwarding to a method to be implemented by the class author.
After our offline discussion, I'll punt this to future work.

Add Langref doc

Fix builders

Fix UB when binding a nullptr as a reference

Rebase after improving diagnostic on invalid IR

rebase and fix build

Alright, I've now looked at everything but OpDefinitionsGen. Will take a look at that one tomorrow.

mlir/docs/LangRef.md
744
mlir/include/mlir/IR/ODSSupport.h
19–21

Should we wrap this in a nested namespace? Is this intended to be called from user code? Feels cleaner to put this in a namespace to add an extra layer of visibility from user code.

mlir/include/mlir/IR/Operation.h
523–524

Can you inline the optional into the if for each of these?

mlir/lib/AsmParser/Parser.cpp
1293–1294
mlir/lib/IR/MLIRContext.cpp
811–812
814

I thought that the optional return was supposed to differentiate nullptr from the name being an inherent attribute, but I don't see any difference here (nullopt is always used for failure).

mlir/test/IR/dynamic.mlir
4 ↗(On Diff #515982)

Why are all of these being deleted?

mlir/test/Rewrite/pdl-bytecode.mlir
3 ↗(On Diff #515982)

Why are all of these tests being deleted? This is extremely concerning to me.

mlir/test/lib/Dialect/Test/TestDialect.cpp
47–48

Why is this necessary?

1833–1834

Same here, why is this necessary?

mlir/test/lib/Dialect/Test/TestDialect.h
63

// -> /// for each of these.

mlir/test/lib/Dialect/Test/TestOps.td
3301–3309

Please use normal strings if you don't need multi-line, it makes the examples much more compact and cleaner.

3345

Can you wrap this at 80?

Fix one more verifier during custom parsing that involves inherent attributes

mehdi_amini marked 11 inline comments as done.Apr 24 2023, 9:54 AM
mehdi_amini added inline comments.
mlir/include/mlir/IR/ODSSupport.h
19–21

I just tried, but this is supposed to work with arbitrary types using ADL.
There are matching the "builtin" Attributes (IntegerAttr and DenseI64ArrayAttr only right now), but one could have their own types.

If I nest these here, I would need ODS to emit a using namespace ::mlir::ods_support; in the generated header and the generated implementation for the op definition, which would be more annoying I think?

mlir/lib/IR/MLIRContext.cpp
814

This is the implementation for unregistered operation: so for these we can't know if there is a missing inherent attr from the DictionaryAttr, so can't return nullptr. We have to assume that if a name isn't in the properties dictionary then there is no inherent attr for this name.

Any other ideas?

Address comments (and rebase)

zero9178 added inline comments.Apr 24 2023, 10:07 AM
mlir/include/mlir/IR/OpBase.td
212

All TableGen code currently uses $_ctxt, this would otherwise be the first use of $_ctx

mehdi_amini marked an inline comment as done.Apr 24 2023, 10:53 AM
martin-luecke added inline comments.
mlir/lib/AsmParser/Parser.cpp
1416
1421

Fix typos reported by Martin

Mogball accepted this revision.Apr 26 2023, 8:15 AM
Mogball added inline comments.
mlir/include/mlir/IR/OpBase.td
198

nit: other tablegen stuff typically uses $_ctxt

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

drop trivial braces

1087

why do some of these have the leading :: and some don't?

This revision is now accepted and ready to land.Apr 26 2023, 8:15 AM
mehdi_amini marked 4 inline comments as done.Apr 26 2023, 8:29 AM
mehdi_amini added inline comments.
mlir/include/mlir/IR/OpBase.td
198

The code was updated, I forgot to fix the doc here.

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

Which ones don't?

All of the mlir:: or llvm:: utilities / symbols should be fully qualified.
Note: Properties is defined by the current class, not by MLIR.

mehdi_amini marked an inline comment as done.

Address Jeff's comments

rriddle accepted this revision.Apr 29 2023, 2:42 AM
Matt added a subscriber: Matt.May 1 2023, 2:02 PM
This revision was automatically updated to reflect the committed changes.