Page MenuHomePhabricator

[mlir] separable registration of attribute and type interfaces
ClosedPublic

Authored by ftynse on Jun 14 2021, 8:22 AM.

Details

Summary

It may be desirable to provide an interface implementation for an attribute or
a type without modifying the definition of said attribute or type. Notably,
this allows to implement interfaces for attributes and types outside of the
dialect that defines them and, in particular, provide interfaces for built-in
types. Provide the mechanism to do so.

Currently, separable registration requires the attribute or type to have been
registered with the context, i.e. for the dialect containing the attribute or
type to be loaded. This can be relaxed in the future using a mechanism similar
to delayed dialect interface registration.

See https://llvm.discourse.group/t/rfc-separable-attribute-type-interfaces/3637

Depends On D104233

Diff Detail

Event Timeline

ftynse created this revision.Jun 14 2021, 8:22 AM
ftynse requested review of this revision.Jun 14 2021, 8:22 AM
ftynse updated this revision to Diff 351889.Jun 14 2021, 8:27 AM

Drop some irrelevant changes

rriddle accepted this revision.Jun 15 2021, 1:10 AM

LG thanks! This has been on the TODO for a while, but just never found the time.

mlir/docs/Interfaces.md
230

Extra newline here.

248

typo: implemenations

265

typo: attirbute

mlir/include/mlir/IR/AttributeSupport.h
67–70

Why return null as opposed to aborting in the lookup(like the other method does)? To provide a better error?

mlir/include/mlir/IR/Attributes.h
34
mlir/include/mlir/IR/StorageUniquerSupport.h
90–91

The documentation here should mention that the type must be registered (i.e. its parent dialect must be loaded) in the context (given that otherwise this aborts).

97

The style of this message doesn't match the other fatal errors, e.g. no capital etc. I can't remember which we prefer for report_fatal_error, but we should be consistent.

mlir/include/mlir/IR/TypeSupport.h
68–79
mlir/include/mlir/IR/Types.h
79–84
mlir/include/mlir/Support/InterfaceSupport.h
223–241

An important invariant about this interface map is that the interfaces can't have state, given that the destructors don't get called. Can we add a validation (I guess is_trivially_destructible) for
the provided interfaces?

mlir/test/lib/Dialect/Test/TestInterfaces.td
66
69
78
88

Is the (ins) here necessary? I would have thought it be defaulted. (same above)

mlir/unittests/IR/InterfaceAttachmentTest.cpp
96–100

I can foresee this being a bit tricky if we want some methods defaulted and some not, but I don't really see a good way to avoid that. (Given that the implementer here generally doesn't have control over the interface implementation.

This revision is now accepted and ready to land.Jun 15 2021, 1:10 AM
ftynse updated this revision to Diff 352079.Jun 15 2021, 3:02 AM

A bit more cmake.

ftynse updated this revision to Diff 352103.Jun 15 2021, 5:17 AM
ftynse marked 15 inline comments as done.

Address feedback.

mlir/include/mlir/IR/AttributeSupport.h
67–70

Provisioning for delayed registration of interfaces like we do for dialects. If lookup returns null, we will be able to store an interface construction lambda for later. In the meantime, this also gives a better error message, the original one refers to attribute/type construction.

mlir/include/mlir/IR/StorageUniquerSupport.h
97

We are really inconsistent about this, I see on average the same amount of capitalized and non-capitalized error message, and only some capitalized error messages have trailing periods. Changed here, but I would consider using the same style as for emitError (no capitalization, no trailing period) for simplicity.

mlir/unittests/IR/InterfaceAttachmentTest.cpp
96–100

It should be possible to provide an ODS mechanism that emits classes with default implementations including/excluding the methods from the list, something like `def
ConcreteName : DefineExternalInterfaceModel<InterfaceName> { let excludeDefaults = ["method1", "method2"]; }` if this becomes common.

Otherwise, I need to dig deeper into both the C++ spec and the specific compiler implementations we care about, but I suspect the method of a template may not be instantiated by the compiler if it is never called. That is, if there is a method in the derived class. Not sure about it, but this would let the user redefine only the methods they need directly in the class deriving from ExternalModel.