Page MenuHomePhabricator

[mlir] enable delayed registration of dialect interfaces
ClosedPublic

Authored by ftynse on Feb 5 2021, 6:41 AM.

Details

Summary

This introduces a mechanism to register interfaces for a dialect without making
the dialect itself depend on the interface. The registration request happens on
DialectRegistry and, if the dialect has not been loaded yet, the actual
registration is delayed until the dialect is loaded. It requires
DialectRegistry to become aware of the context that contains it and the context
to expose methods for querying if a dialect is loaded.

This mechanism will enable a simple extension mechanism for dialects that can
have interfaces defined outside of the dialect code. It is particularly helpful
for, e.g., translation to LLVM IR where we don't want the dialect itself to
depend on LLVM IR libraries.

Diff Detail

Event Timeline

ftynse created this revision.Feb 5 2021, 6:41 AM
ftynse requested review of this revision.Feb 5 2021, 6:41 AM

I was discussing a bit with River and we think we can avoid the back pointer to the context by restoring proper encapsulation by removing the getDialectRegistry() from MLIRContext, let me try to prepare a patch that does that.

mlir/include/mlir/IR/Dialect.h
280

I'd expect to see an "append" of the interfaces, it seems like you're replacing the ones in the destination here.

mlir/include/mlir/IR/MLIRContext.h
63 ↗(On Diff #321745)

I'm not sure these API bear their weight here, can we just rely on getLoadedDialect?

This lead to pattern of double lookup, like you do in DialectRegistry:

if (owningContext->isDialectLoaded(dialectName))
  Dialect *dialect = owningContext->getLoadedDialect(dialectName);
ftynse updated this revision to Diff 322313.Feb 9 2021, 1:51 AM
ftynse marked 2 inline comments as done.

Address review

ftynse added a comment.Feb 9 2021, 1:53 AM

I was discussing a bit with River and we think we can avoid the back pointer to the context by restoring proper encapsulation by removing the getDialectRegistry() from MLIRContext, let me try to prepare a patch that does that.

It looks quite widely used, so may be worth having a separate cleanup patch for that.

ftynse added a comment.Feb 9 2021, 4:27 AM

The child commit https://reviews.llvm.org/D96331 makes getDialectRegistry return a const reference instead.

mehdi_amini accepted this revision.Feb 9 2021, 10:00 AM

LG with the child.

This revision is now accepted and ready to land.Feb 9 2021, 10:00 AM
This revision was landed with ongoing or failed builds.Feb 10 2021, 3:07 AM
This revision was automatically updated to reflect the committed changes.