In this CL, update the function name of verifier according to the
behavior. If a verifier needs to access the region then it'll be updated
to verifyRegions.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
mlir/include/mlir/Interfaces/InferTypeOpInterface.td | ||
---|---|---|
67–68 | ||
mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp | ||
1780 | Some of the checks here are not related to regions. We should really keep the checks contained to where they should be checked (I would keep everything non-region in the main hasVerifier). hasRegionVerifier should really only contain region related verification code. | |
mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp | ||
1039 | Same comment here and throughout. | |
mlir/lib/IR/Operation.cpp | ||
1090–1091 | If it should be well formed (i.e. if you are guaranteed to be able to assume it), we should just remove the assert. It feels like a legacy assert at this point. | |
mlir/test/Dialect/Tensor/invalid.mlir | ||
379 | Missing newline here? |
Address review comment and quickly went through the verifiers on Ops and updated some of them.
mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp | ||
---|---|---|
1780 | Supported both hasVerifier and hasRegionVerifier and split the verifier based on if it touches the region | |
mlir/lib/IR/Operation.cpp | ||
1090–1091 | Ideally no one should just use this except the trait. Removed. | |
mlir/test/Dialect/Tensor/invalid.mlir | ||
379 | Not sure what happened here, upload a new one and see if this is still happening. |
Nice thanks
mlir/include/mlir/IR/OpBase.td | ||
---|---|---|
2076–2077 | This probably becomes more interesting with SameOperandsAndResultType moving to to interfaces (and I still need to do some work there to make it OpTraitList), A little bit of artifact of this file being a collection of all attributes. | |
mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp | ||
2161 | I'm a little bit hesitant to have this duplicated at every decl site. | |
mlir/test/Dialect/GPU/invalid.mlir | ||
29 | Ooc why this change? | |
mlir/test/Dialect/LLVMIR/global.mlir | ||
175 | Having the SSA id in the error message is unfortunate, especially as not guaranteed to be stable (so makes it easy to create a flaky test here, e.g., every run could have a different id returned) | |
mlir/test/Dialect/SCF/invalid.mlir | ||
292 | So here yield would fail first to verify parent relationship and change error else? |
mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp | ||
---|---|---|
2161 | Yeah, can we just drop this? Feels like this should be gleaned from the ODS documentation (which I know right now is a mess). | |
mlir/lib/Dialect/SPIRV/IR/SPIRVOps.cpp | ||
4111 | Can we signal this in the op definition? i.e. by removing the hasVerifier or by adding hasVerifier = ?(if the hasVerifier was set in a base class). |
Address review comments and rebase
mlir/include/mlir/IR/OpBase.td | ||
---|---|---|
2076–2077 | Yep, it may be a problem for describing dependency between these traits and interfaces. Not sure if we can do it in pure string rather than a tblgen def | |
mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp | ||
2161 | All removed. | |
mlir/lib/Dialect/SPIRV/IR/SPIRVOps.cpp | ||
4111 | disable them in the derived class by adding let hasVerifier = 0 | |
mlir/test/Dialect/GPU/invalid.mlir | ||
29 | return expects its parent to be builtin.func so that we won't get the expected error. In fact, there are many cases like this, most of the ops are well-defined, i.e., they specify the restriction of terminator, the restriction of parent op, .etc. As a result, some tests case we may not be able to verify the fact they want without using ops from other dialects(like test dialect) | |
mlir/test/Dialect/LLVMIR/global.mlir | ||
175 | You're right. Removed the later of "block with no terminator" | |
mlir/test/Dialect/SCF/invalid.mlir | ||
292 | Yes, like the case above. yield has assigned its parent |
mlir/lib/Dialect/GPU/IR/GPUDialect.cpp | ||
---|---|---|
278 | Can we remove this? |
This probably becomes more interesting with SameOperandsAndResultType moving to to interfaces (and I still need to do some work there to make it OpTraitList), A little bit of artifact of this file being a collection of all attributes.