[PATCH] D76329: [MLIR] Deduplicate dialect registration by ClassID

[River Riddle via Phabricator via llvm-commits]

rriddle accepted this revision.
rriddle added a comment.
This revision is now accepted and ready to land.

In D76329#1928036 <https://reviews.llvm.org/D76329#1928036>, @GMNGeoffrey wrote:

> In D76329#1927997 <https://reviews.llvm.org/D76329#1927997>, @mehdi_amini wrote:
>
> > 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.



================
Comment at: mlir/lib/IR/Dialect.cpp:29
 
 // Registry for all dialect allocation functions.
+static llvm::ManagedStatic<DenseMap<const ClassID *, DialectAllocatorFunction>>
----------------
Can you change these to /// while you are here?


================
Comment at: mlir/lib/IR/Dialect.cpp:30
 // Registry for all dialect allocation functions.
-static llvm::ManagedStatic<SmallVector<DialectAllocatorFunction, 8>>
+static llvm::ManagedStatic<DenseMap<const ClassID *, DialectAllocatorFunction>>
     dialectRegistry;
----------------
Can you change this to a MapVector or other stable container? DenseMap does not have a stable insertion order.


================
Comment at: mlir/lib/IR/Dialect.cpp:39
 /// used through the DialectRegistration template.
-void mlir::registerDialectAllocator(const DialectAllocatorFunction &function) {
+void mlir::registerDialectAllocator(const ClassID *classId,
+                                    const DialectAllocatorFunction &function) {
----------------
Can you make this a private method inside of `Dialect` that is only accessible via `registerDialect`?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76329/new/

https://reviews.llvm.org/D76329