Page MenuHomePhabricator

[mlir] Refactor how additional verification is specified in ODS
ClosedPublic

Authored by rriddle on Feb 1 2022, 3:02 PM.

Details

Summary

Currently if an operation requires additional verification, it specifies an inline
code block (let verifier = "blah"). This is quite problematic for various reasons, e.g.
it requires defining C++ inside of Tablegen which is discouraged when possible, but mainly because
nearly all usages simply forward to a static function static LogicalResult verify(SomeOp op).
This commit adds support for a hasVerifier bit field that specifies if an additional verifier
is needed, and when set to 1 declares a LogicalResult verify() method for operations to
override. For migration purposes, the existing behavior is untouched. Upstream usages will
be replaced in a followup to keep this patch focused on the hasVerifier implementation.

One main user facing change is that what was once MyOp::verify is now MyOp::verifyInvariants.
This better matches the name this method is called everywhere else, and also frees up verify for
the user defined additional verification. The verify function when generated now (for additional
verification) is private to the operation class, which should also help avoid accidental usages after
this switch.

Diff Detail

Event Timeline

rriddle created this revision.Feb 1 2022, 3:02 PM
rriddle requested review of this revision.Feb 1 2022, 3:02 PM
rriddle edited the summary of this revision. (Show Details)Feb 1 2022, 3:47 PM

In D116789, I'm considering to use a trait to specify if an operation has an additional verifier. There are two kind of the these traits, indicating when it'll be run(before/after region verification). What do you think about this approach? Sorry that CL is pending now, will work on it ASAP.

In D116789, I'm considering to use a trait to specify if an operation has an additional verifier. There are two kind of the these traits, indicating when it'll be run(before/after region verification). What do you think about this approach? Sorry that CL is pending now, will work on it ASAP.

Thanks for pinging that, commented on that patch.

lattner accepted this revision.Feb 2 2022, 8:25 AM
lattner added a subscriber: lattner.

This is great River, it reduces boilerplate etc. That said, I particularly like how this nudges people towards a consistent approach, rather than everyone making up their own similar-but-different patterns.

Thank you for doing this, do you think parser/printer could follow a similar approach?

This revision is now accepted and ready to land.Feb 2 2022, 8:25 AM

This is great River, it reduces boilerplate etc. That said, I particularly like how this nudges people towards a consistent approach, rather than everyone making up their own similar-but-different patterns.

Thank you for doing this, do you think parser/printer could follow a similar approach?

Yeah, I intend to fix parser/printers in a followup.

mehdi_amini added inline comments.Feb 2 2022, 2:08 PM
mlir/include/mlir/IR/OpDefinition.h
204

The comment can be misleading for this now.

mlir/lib/Target/SPIRV/Serialization/Serializer.cpp
89

shouldn't this be instead verify(module)?