By default, the verifier won't allow operations with unregistered
dialect. A flag can be set on the context to allow this and a
command line option is provided for mlir-opt to opt-in.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Time | Test | |
---|---|---|
30 ms | MLIR.IR::Unknown Unit Message ("") | |
50 ms | MLIR.IR::Unknown Unit Message ("") |
Event Timeline
mlir/include/mlir/IR/MLIRContext.h | ||
---|---|---|
53 | These sound weird, can we use the same wording as the options for operations/types in dialects? allowsUnregisteredDialects()/allowUnregisteredDialects(bool allow)? | |
mlir/lib/Analysis/Verifier.cpp | ||
209 | Seems like you should put the check on this back-edge to avoid redundant lookups. | |
mlir/lib/IR/MLIRContext.cpp | ||
168 | Please use /// for comments. | |
mlir/lib/Support/MlirOptMain.cpp | ||
78 | Using the suggestion above would simplify this to just: context.allowUnregisteredDialects(allowUnregisteredDialect). | |
mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp | ||
449 | I actually would have expected this one to be fine, but it seems that OperationName::getDialect doesn't properly handle unregistered operations for the builtin dialect(no namespace). I think that is fine though. | |
mlir/tools/mlir-translate/mlir-translate.cpp | ||
49 | This looks unused. | |
mlir/unittests/IR/OperationSupportTest.cpp | ||
18 | Can you move this out of the namespace and mark as static while you are here? |
mlir/lib/IR/MLIRContext.cpp | ||
---|---|---|
168 | I had modeled on the data members above : // Identifier allocator and mutex for thread safety. llvm::BumpPtrAllocator identifierAllocator; llvm::sys::SmartRWMutex<true> identifierMutex; |
The commit summary opening "By default, the verifier won't allow operations with unregistered dialect. " reads as if this was the situation before. Consider rephrasing please. "Change the verifier to disallow ..."? Besides "operations with unregistered dialect", what about unregistered operations, i.e., operations that aren't part of any registered dialect? It looks like you are equating the two, and did you mean to use the latter for this patch?
mlir/tools/mlir-opt/mlir-opt.cpp | ||
---|---|---|
88 | Nit: "operation for unregistered dialects" isn't really meaningful. Rephrase here and other places. | |
167 | clAllowUnregisteredDialects ? |
I used the future tense here to indicate the change.
Besides "operations with unregistered dialect", what about unregistered operations, i.e., operations that aren't part of any registered dialect?
To me “operations with unregistered dialect” is the same as “ operations that aren't part of any registered dialect”.
“Unregistered operations” on the other hand can refer to an operation with a known dialect but that isn’t registered within the dialect. There is already a per-dialect flag to control this.
mlir/tools/mlir-opt/mlir-opt.cpp | ||
---|---|---|
88 | I can’t think of anything more descriptive, please suggest an alternative if you have one | |
167 | Is this a documented or widespread convention? |
But the two are clearly different. There could be an operation that is part of a dialect that is not registered, and you then could have operations that aren't part of any dialect like "foo"() : () -> () -- the latter isn't part of say any dialect.
“Unregistered operations” on the other hand can refer to an operation with a known dialect but that isn’t registered within the dialect. There is
already a per-dialect flag to control this.
If the dialect itself hasn't been registered with <whatever we are referring to>, then obviously its ops themselves aren't registered. I think you could consistently use "ops with no / unregistered dialects" everywhere to be accurate.
mlir/tools/mlir-opt/mlir-opt.cpp | ||
---|---|---|
88 | Allow operations with no/unregistered dialects ? | |
167 | I thought it was a convention - unless folks started deviating from it. Readability-wise IMO it's a win because it's easily distinguishable as a cmd line flag, and you would in many cases want the same name with cl dropped somewhere in that pass. |
OK I see what you mean, I'll write “operations that aren't part of any registered dialect” which covers both f.oo which if part of the unregistered dialect f and foo which is not part of any dialect. The verifier is covering both here.
(I don't even know why we allow such operation name in the first place to be honest, other than saving a few chars when writing tests there isn't much interest)
mlir/tools/mlir-opt/mlir-opt.cpp | ||
---|---|---|
167 | The other variables above: passPipeline, splitInputFile, verifyDiagnostics, and verifyPasses are all cl flags and not using this convention, so I just followed what is there at the moment. I don't mind the cl prefix, but I'd rather apply it consistently. |
This is wrong. Operations are always part of a dialect. "foo" would be considered to be part of the builtin dialect, which currently has no namespace. IMO we should set the namespace of the builtin dialect to "mlir" and the assert that operation names
always have a dialect prefix.
Do you know why I see this?
echo '"foo"() : () -> ()' | ./bin/mlir-opt <stdin>:1:1: error: 'foo' op created with unregistered dialect. If this is intended, please call allowUnregisteredDialects() on the MLIRContext, or use -allow-unregistered-dialect with mlir-opt
IMO we should set the namespace of the builtin dialect to "mlir" and the assert that operation names always have a dialect prefix.
I'd prefer this indeed.
mlir/lib/Analysis/Verifier.cpp | ||
---|---|---|
216 | I usually don't try to optimize for out-of-tree users, they'll interpret however it makes sense for them (or they can patch the message if they want). |
I didn't know the unknown named ops were considered as part of the builtin dialect - they are anything but built-in!
mlir/tools/mlir-opt/mlir-opt.cpp | ||
---|---|---|
167 | Sounds good, thanks. |
Because OperationName::getDialect does not account for the fact that a prefix may be missing. It(IMO rightfully) assumes that all operations have a dialect namespace prefix. I wrote it that way because I'd always intended to make the builtin dialect have a proper namespace, but forgot about it until now. No one checks for the dialect prefix of the builtin operations ('func'/'module'/etc.) so it has just "worked" up until now.
One advantage has been in writing test cases where you need an op with a side-effect so that it just doesn't become dead code (when canonicalization is being run as part of the pass/utility you are testing). These unknown ops provided the easiest/shortest IR for it - we would have otherwise had to think of another side-effecting op (like creating a memref type and a load/store on it). Any other quick shortcut replacements?
Another use case of allowing such unknown ops is a need to continue / allow transformation in their presence before the output goes to another tool that is able to finally deal with those ops. Why force a dialect to be registered when an mlir-opt transform can meaningfully work in the presence of a mix of registered and unregistered ops?
Using a name with a dot?
foo.myop instead of ˋmyop` should give the same effect I expect.
(Could also use the test dialect to avoid using the new flag moving forward)
Another use case of allowing such unknown ops is a need to continue / allow transformation in their presence before the output goes to another tool that is able to finally deal with those ops. Why force a dialect to be registered when an mlir-opt transform can meaningfully work in the presence of a mix of registered and unregistered ops?
mlir-opt is really a testing tool, I expect any « real » compiler built with MLIR to always register their dialects.
You mean 'foo' is a registered dialect? Won't you still still get an error if 'myop' isn't registered and we disallow all unregistered ops. Or you were suggesting something else?
(Could also use the test dialect to avoid using the new flag moving forward)
Sure.
Another use case of allowing such unknown ops is a need to continue / allow transformation in their presence before the output goes to another tool that is able to finally deal with those ops. Why force a dialect to be registered when an mlir-opt transform can meaningfully work in the presence of a mix of registered and unregistered ops?
mlir-opt is really a testing tool, I expect any « real » compiler built with MLIR to always register their dialects.
Even for testing, I meant if one wanted to run:
.... | mlir-opt ... | <another-mlir-based-tool> ...
If mlir-opt errors out on unregistered dialect operations, one can't test a mix of mlir registered dialect ops and other ops mid-way in the pipeline there.
Some context got lost I guess, you were answering to "(I don't even know why we allow such operation name in the first place to be honest, other than saving a few chars when writing tests there isn't much interest)" which referred to ops that don't contain any dot in the name.
mlir-opt is really a testing tool, I expect any « real » compiler built with MLIR to always register their dialects.
Even for testing, I meant if one wanted to run:
.... | mlir-opt ... | <another-mlir-based-tool> ...If mlir-opt errors out on unregistered dialect operations, one can't test a mix of mlir registered dialect ops and other ops mid-way in the pipeline there.
Right, but that's why the command line flag is provided.
These sound weird, can we use the same wording as the options for operations/types in dialects? allowsUnregisteredDialects()/allowUnregisteredDialects(bool allow)?