This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Support verification order (3/3)
ClosedPublic

Authored by Chia-hungDuan on Feb 22 2022, 5:13 PM.

Details

Summary

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.

Diff Detail

Event Timeline

Chia-hungDuan created this revision.Feb 22 2022, 5:13 PM
Chia-hungDuan requested review of this revision.Feb 22 2022, 5:13 PM
rriddle added inline comments.Feb 25 2022, 11:33 AM
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?

rriddle requested changes to this revision.Feb 25 2022, 11:33 AM
This revision now requires changes to proceed.Feb 25 2022, 11:33 AM
Chia-hungDuan marked 4 inline comments as done.

Address review comment and quickly went through the verifiers on Ops and updated some of them.

Chia-hungDuan added inline comments.Feb 28 2022, 7:05 PM
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.

Fix verification label in linalg op interface

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?

Herald added a project: Restricted Project. · View Herald TranscriptMar 2 2022, 10:48 AM
rriddle added inline comments.Mar 6 2022, 8:52 PM
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).

Chia-hungDuan marked 4 inline comments as done.

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.
Added more descriptions in the doc.

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

rriddle accepted this revision.Mar 10 2022, 12:32 PM
rriddle added inline comments.
mlir/lib/Dialect/GPU/IR/GPUDialect.cpp
278

Can we remove this?

This revision is now accepted and ready to land.Mar 10 2022, 12:32 PM

Address review comment

mlir/lib/Dialect/GPU/IR/GPUDialect.cpp
278

Sorry, a leftover.

This revision was automatically updated to reflect the committed changes.