Page MenuHomePhabricator

[mlir] Refactor DialectRegistry delayed interface support into a general DialectExtension mechanism
ClosedPublic

Authored by rriddle on Feb 22 2022, 4:19 PM.

Details

Summary

The current dialect registry allows for attaching delayed interfaces, that are added to attrs/dialects/ops/etc.
when the owning dialect gets loaded. This is clunky for quite a few reasons, e.g. each interface type has a
separate tracking structure, and is also quite limiting. This commit refactors this delayed mutation of
dialect constructs into a more general DialectExtension mechanism. This mechanism is essentially a registration
callback that is invoked when a set of dialects have been loaded. This allows for attaching interfaces directly
on the loaded constructs, and also allows for loading new dependent dialects. The latter of which is
extremely useful as it will now enable dependent dialects to only apply in the contexts in which they
are necessary. For example, a dialect dependency can now be conditional on if a user actually needs the
interface that relies on it.

Diff Detail

Event Timeline

rriddle created this revision.Feb 22 2022, 4:19 PM
rriddle requested review of this revision.Feb 22 2022, 4:19 PM
ormris removed a subscriber: ormris.Feb 24 2022, 10:07 AM

This is very elegant! The combination of the trick for converting lambda to bare function pointer and deduce the Dialect dependency is quite interesting :)

mlir/include/mlir/IR/DialectRegistry.h
89

It wasn't obvious to me that this parameter wasn't used: can you comment it out maybe? Or name it unusedDialects?

142

Update the comment? Register all dialects and extensions ...

143

This API does not take a context anymore, the comment is stalled on this as well.

189

This is a neat trick!

204

The dtor declaration does not seem necessary here, is it?

212

I don't find any place where addExtension is called with a lambda with more than a single dialect?

mlir/lib/IR/Dialect.cpp
192

Can't you just have a for-range loop here?

202

You're building the requiredDialects for the general case (DialectExtensionBase), but actually the only implementation DialectExtension will ignore this and rebuild it entirely.
Do you anticipate other implementations of DialectExtensionBase that would use it?

Herald added a project: Restricted Project. · View Herald TranscriptMar 14 2022, 9:51 PM
mehdi_amini accepted this revision.Mar 14 2022, 9:51 PM

LG with inline comments!

This revision is now accepted and ready to land.Mar 14 2022, 9:51 PM