Page MenuHomePhabricator

[MLIR] Deduplicate dialect registration by ClassID
ClosedPublic

Authored by GMNGeoffrey on Mar 17 2020, 2:47 PM.

Details

Summary

With the move towards dialect registration that does not depend only use
static initialization, we are running into more cases where the dialects
are registered by different methods. For example, TensorFlow still uses
static initialization to register all MLIR core dialects, which prevents
explicit registration of any of them when linking it in. We ran into this
issue in https://github.com/google/iree/pull/982.

To address potential issues with conflicts from non-standard
allocators passed to registerDialectAllocator, made this method
private. Now all dialects can only be registered with their
constructor.

Similarly deduplicates DialectHooks for consistency and makes their
registration follow the same pattern.

Diff Detail

Event Timeline

GMNGeoffrey created this revision.Mar 17 2020, 2:47 PM
Herald added a project: Restricted Project. · View Herald Transcript
mehdi_amini accepted this revision.Mar 17 2020, 5:59 PM

LGTM, but I'd like to hear from River as well.

I am not sure what you mean with:

This potentially introduces some issues if callers using non-standard dialect registration functions passed to registerDialectAllocator and they are silently overridden.

LGTM, but I'd like to hear from River as well.

Yeah I'd also like Rivers' review

I am not sure what you mean with:

This potentially introduces some issues if callers using non-standard dialect registration functions passed to registerDialectAllocator and they are silently overridden.

If people call registerDialect<Foo>() we're all good as if there's a duplicate, it would have been the same thing anyway. If someone is directly calling registerDialectAllocator (which I don't think anyone is right now) and they pass in their custom allocator that they expect to do something and someone else also registers the same dialect (either with registerDialect or registerDialectAllocator) then we might have surprising behavior. We could move registerDialectAllocator out of the public API, although it's used by the template function, so a bit tricky (maybe just an impl/detail namespace?), but I wanted to hear from others before doing that.

rriddle accepted this revision.Mar 17 2020, 10:07 PM

LGTM, but I'd like to hear from River as well.

Yeah I'd also like Rivers' review

I am not sure what you mean with:

This potentially introduces some issues if callers using non-standard dialect registration functions passed to registerDialectAllocator and they are silently overridden.

If people call registerDialect<Foo>() we're all good as if there's a duplicate, it would have been the same thing anyway. If someone is directly calling registerDialectAllocator (which I don't think anyone is right now) and they pass in their custom allocator that they expect to do something and someone else also registers the same dialect (either with registerDialect or registerDialectAllocator) then we might have surprising behavior. We could move registerDialectAllocator out of the public API, although it's used by the template function, so a bit tricky (maybe just an impl/detail namespace?), but I wanted to hear from others before doing that.

In general I'm fine with this, as it makes some things easier(we can now make getRegisteredDialect O(1)(modulo locking)). The only problem I can foresee is with the allocator, as you describe. IMO we can just hide that method for now.

mlir/lib/IR/Dialect.cpp
29–30

Can you change these to /// while you are here?

30–32

Can you change this to a MapVector or other stable container? DenseMap does not have a stable insertion order.

39–40

Can you make this a private method inside of Dialect that is only accessible via registerDialect?

This revision is now accepted and ready to land.Mar 17 2020, 10:07 PM
GMNGeoffrey marked 2 inline comments as done.

Stable maps and private registration.

Use stable-ordered maps. Make allocator functions private static methods.
Make dialect and dialect hooks registry have the same format.

GMNGeoffrey marked 2 inline comments as done.Mar 18 2020, 5:25 PM
GMNGeoffrey added inline comments.
mlir/lib/IR/Dialect.cpp
39–40

Made this and the corresponding hooks setter registration private static methods on the class with friend declarations. Hopefully that's what you meant. In the process, made the registration for hooks and dialects the same format (separate the free template function and the global initialization struct).

GMNGeoffrey edited the summary of this revision. (Show Details)Mar 18 2020, 5:27 PM
GMNGeoffrey marked an inline comment as done.

River if this LGTY, could you please commit it?

Looks like the failure is a clang format diff. I think this is related to the discussion in https://llvm.discourse.group/t/remove-mlir-custom-clang-format-configuration/647/4? I can reformat as needed, but +1 for unifying LLVM and MLIR configs

Break templates declaration to placate clang format pre-merge.

This revision was automatically updated to reflect the committed changes.