[mlir] Add method to populate default attributes

[jpienaar]

Summary

Add hook to populate default attributes and generate method for ODS ops.
Previously default attributes were only usable by way of the ODS generated
accessors, but this was undesirable as

1. The ODS getters could construct Attribute each get request;
2. For non-C++ uses this would require either duplicating some of the default attribute generating or generating additional bindings to generate methods;
3. Accessing op.getAttr("foo") and op.getFoo() would return different results;

The hook can be used to address these.

This merely adds this facility but does not employ by default on any path.

[rriddle]

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

1595–1601

Can we avoid generating the method at all if we don't need to populate it?

1615

If this method also took the OperationName, you could use the cached StringAttrs for the attribute names instead of using the raw string.

[jpienaar]

Pass in OperationName and use cached StringAttrs.

[jpienaar]

(I think I can avoid passing in the context, will revise)

[jpienaar]

Switched to OperationName but kept MLIRContext as input. Context could be
avoided but for unregistered ops it becomes indirect/unituitive (and
restricting to registered is unnecessary).

[rriddle]

In what cases would these methods get called by an unregistered operation? i.e. when does the MLIRContext come into play?

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

1611

Should we just have the input opName be the registered version? Also OperationName is pointer-like, so you don't really need to use const &.

1614

nit: Drop the braces here.

[jpienaar]

In D128962#3632093, @rriddle wrote:

In what cases would these methods get called by an unregistered operation? i.e. when does the MLIRContext come into play?

I was thinking where you'd have ops relying on dialect fallback hook (so dialect is registered, dialect allows for unregistered ops and op isn't). Except where dialect of op is not registered we'd be able to query the context on the dialect of the OperationName. The dialect example af start was only case with unregistered op which I thought was useful, for unregistered dialects I can't really imagine (but that's also as I almost never have those except for pure testing). So we can remove it if we don't care about unregistered dialects, folks would just need to recall that they can query the context on the dialect.

[jpienaar]

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

1611

Good point about pointer, so only case to discuss is the unregistered op of registered dialect case that would push this one way or another. (Will update once at desk)

[jfurtek]

I think that this has the potential to improve Python bindings for operations with default-valued attributes. (I believe that at the moment, in most cases the generated Python op creation code requires a value to be provided for an attribute that has a default value.)

I am wondering, though, if this is the best approach. This seems to create a "two phase" construction for operations: the operation is created, and then later (hopefullly!) the default attributes are set. So, for example, a custom builder for an operation with a default-valued attribute doesn't really have all of the "information" - that won't happen until the default attributes of the operation are populated.

An alternative approach might be to have a function that populates a named attribute list given a registered operation name, and this list could be passed to the builder that takes an arrayref of named attributes. (This might even be more efficient in situations where the attribute list could be reused to create multiple operations.)

Maybe I'm drawing too much on how C++ works, or maybe I'm stuck in the Attribute/Type mode of thinking (where creation via getChecked() can fail). Just throwing this out as an idea...

[rriddle]

In D128962#3632691, @jpienaar wrote:

In D128962#3632093, @rriddle wrote:

In what cases would these methods get called by an unregistered operation? i.e. when does the MLIRContext come into play?

I was thinking where you'd have ops relying on dialect fallback hook (so dialect is registered, dialect allows for unregistered ops and op isn't). Except where dialect of op is not registered we'd be able to query the context on the dialect of the OperationName. The dialect example af start was only case with unregistered op which I thought was useful, for unregistered dialects I can't really imagine (but that's also as I almost never have those except for pure testing). So we can remove it if we don't care about unregistered dialects, folks would just need to recall that they can query the context on the dialect.

These methods aren't called by unregistered operations though (given the use within on RegisteredOperationName)? So do we need the context? or are you thinking of some extension here?

[jpienaar]

In D128962#3633055, @jfurtek wrote:

I think that this has the potential to improve Python bindings for operations with default-valued attributes. (I believe that at the moment, in most cases the generated Python op creation code requires a value to be provided for an attribute that has a default value.)

Indeed, that is one of the goals (as the special casing there feels bad).

I am wondering, though, if this is the best approach. This seems to create a "two phase" construction for operations: the operation is created, and then later (hopefullly!) the default attributes are set. So, for example, a custom builder for an operation with a default-valued attribute doesn't really have all of the "information" - that won't happen until the default attributes of the operation are populated.

An alternative approach might be to have a function that populates a named attribute list given a registered operation name, and this list could be passed to the builder that takes an arrayref of named attributes. (This might even be more efficient in situations where the attribute list could be reused to create multiple operations.)

Maybe I'm drawing too much on how C++ works, or maybe I'm stuck in the Attribute/Type mode of thinking (where creation via getChecked() can fail). Just throwing this out as an idea...

That is what is exposed on the RegisteredOperation (populating a NamedAttributeList), so that we can integrate this with the builder and run before the op is created. But you are correct that that isn't exposed C API side that way. So you'd have a more efficient way of doing this C++ side but not C side. I was planning on making this default internally to Python C++ code, so that Python side this would always be the behavior and even for all ODS generated methods. I had considered default for all create calls ... but undecided there as I would want to leave option for where caller knows that all attributes have been populated and so no checking is needed (I think that cost isn't significant enough to special case but would leave that discussion for then :))

[jpienaar]

In D128962#3633799, @rriddle wrote:

In D128962#3632691, @jpienaar wrote:

In D128962#3632093, @rriddle wrote:

In what cases would these methods get called by an unregistered operation? i.e. when does the MLIRContext come into play?

I was thinking where you'd have ops relying on dialect fallback hook (so dialect is registered, dialect allows for unregistered ops and op isn't). Except where dialect of op is not registered we'd be able to query the context on the dialect of the OperationName. The dialect example af start was only case with unregistered op which I thought was useful, for unregistered dialects I can't really imagine (but that's also as I almost never have those except for pure testing). So we can remove it if we don't care about unregistered dialects, folks would just need to recall that they can query the context on the dialect.

These methods aren't called by unregistered operations though (given the use within on RegisteredOperationName)? So do we need the context? or are you thinking of some extension here?

You are correct, I somehow forgot that I'm putting this effectively as restriction already and that if I wanted a different case an extension would be needed. Sold, I'll revert back to using RegisterOperationName and removing context.