Dynamic dialects are dialects that can be defined at runtime.
Dynamic dialects are extensible by new operations, types, and
attributes at runtime.
Details
- Reviewers
rriddle mikeurbach nicolasvasilache - Commits
- rGba8424a251d7: [mlir] Add Dynamic Dialects
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
I would like to add tests for dynamic dialects, by creating a new dynamic dialect, and then populating it with dynamic operations, types, and attributes.
However, the way mlir-opt currently works is by creating a DialectRegistry, and then passing it to MlirOptMain.
Is there any way of getting somewhere a MLIRContext to register the dialect?
Alternatively, how should we register DynamicDialect from DialectRegistry, since we want to directly access them to add operations/types/attributes?
I think for the registry we could have a different type of allocator function that we wrap into the normal dialect allocator function. That way we could do
something like:
registry.insertDynamic("name", [](DynamicDialect *dialect) { // Insert the stuff for the dialect here. });
Inside of the registry though, we should remove the need to create a TypeID upfront for dynamic dialects. Either by using a different registry map, or by using something like TypeID::get<void>() internally. Given that dynamic dialects don't have the same TypeID problems that normal dialects have, the TypeID in the registry doesn't really guard against much IMO.
mlir/include/mlir/IR/ExtensibleDialect.h | ||
---|---|---|
563 | Is this explicit initialization necessary? | |
mlir/include/mlir/IR/MLIRContext.h | ||
117–118 | This should likely be named getOrLoadDynamicDialect to match the other API. With that, why do we need to abort if a dialect of the same name is already loaded? | |
mlir/lib/IR/ExtensibleDialect.cpp | ||
558 | Diagnostic messages should start with a lower case. Same above. | |
mlir/lib/IR/MLIRContext.cpp | ||
460 | If we are actually loading the dialect, we could copy the string into abstractDialectSymbolAllocator or something. That would remove a lot of the uniquing cost. Also, can you assert that we aren't in a multithreaded context here? |
I added an insertDynamic in the DialectRegistry.
My dynamic.mlir test is currently failing (it seems because of a memory issue), and I'll debug this tomorrow.
mlir/include/mlir/IR/MLIRContext.h | ||
---|---|---|
117–118 | I think I'll change it so it returns nullptr instead of aborting. The problem with a getOrLoadDynamicDialect is that we wouldn't know if the dialect was created or not. Does that make sense? | |
mlir/lib/IR/MLIRContext.cpp | ||
460 | Is this what you meant about abstractDialectSymbolAllocator? Also, for the multithreading context, adding an assert here seems to fail on my test. Is there some place where I should deactivate it when loading a new dynamic dialect with the DialectRegistry during parsing? |
mlir/include/mlir/IR/MLIRContext.h | ||
---|---|---|
117–118 | Can we just do what we do in the normal case? i.e. pass in the initializer function and only call it if the dialect needs to be constructed? | |
mlir/lib/IR/Dialect.cpp | ||
180 | Can we move nameStr here? or do something like nameStr = name.str() in the capture list? | |
mlir/lib/IR/MLIRContext.cpp | ||
460 | That feels weird, we can only load a dialect in a non-multithreaded context. We are already asserting in getOrLoadDialect, but we should also assert here as well (just to make sure the assert context is more accurate) | |
mlir/test/lib/Dialect/TestDyn/TestDynDialect.cpp | ||
20 | Can we keep this in the test namespace? I don't think there is a good reason to diverge. | |
27 | Could you move the functors out-of-line and the inline the call to DynamicOpDefinition::get? | |
30–32 | ||
mlir/test/lib/Dialect/TestDyn/TestDynDialect.h | ||
1 ↗ | (On Diff #428836) | This header shouldn't be necessary, it isn't included anywhere. |
This should now be ready for review.
I added documentation for the dynamic dialect definition, and addressed the comments.
Nice.
mlir/docs/DefiningDialects.md | ||
---|---|---|
396 | ||
mlir/include/mlir/IR/DialectRegistry.h | ||
140–149 | Can you reflow these comments? | |
mlir/include/mlir/IR/ExtensibleDialect.h | ||
558–559 | Can you reflow these comments? | |
mlir/include/mlir/IR/MLIRContext.h | ||
114 | ||
117 | Can we pass by pointer here? Dialects are generally interacted with by-pointer, so it would be nice to be consistent here. | |
mlir/lib/IR/Dialect.cpp | ||
173–174 | Can you reflow the comments here? | |
176 | ||
183 | Is this to make sure the lambda returns the right type? Can you just mark the type in the lambda declaration? i.e. ...) -> Dialect * { | |
mlir/lib/IR/MLIRContext.cpp | ||
467 | The llvm:: shouldn't be necessary, can you drop it? | |
486 | Can you just pass dialect by-value? It's a pointer so the reference doesn't matter. | |
1010–1011 | This feels unrelated. Same with the comment changes above. | |
mlir/test/IR/dynamic.mlir | ||
137 | Super nit: can you drop the double newline? | |
142 | Missing newline? | |
mlir/test/lib/Dialect/TestDyn/TestDynDialect.cpp | ||
20–21 | ||
22 | Can we drop the ::mlir:: on these? Same with the registry. |
Sorry for the delay, I forgot a bit about this patch!
@rriddle, I just rebased it, could you land this for me, I do not have commit access?