Page MenuHomePhabricator

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

Authored by Chia-hungDuan on Dec 6 2021, 12:38 AM.

Details

Summary

This CL supports adding dependency between traits verifiers and the
dependency will be checked statically.

Diff Detail

Event Timeline

Chia-hungDuan created this revision.Dec 6 2021, 12:38 AM
Chia-hungDuan requested review of this revision.Dec 6 2021, 12:38 AM

Upload a WIP CL for showing the concept I want to do, let's have discussion based on this change.

jpienaar added inline comments.Dec 10 2021, 7:49 AM
mlir/include/mlir/IR/OpBase.td
2187–2188

Could we make traits more self-documenting?

mlir/include/mlir/Support/InterfaceSupport.h
71 ↗(On Diff #391984)

I'd rather prefer we generate the static asserts from ODS when generating interfaces than add to the class template (previously we havent had need to generate traits as there was not much more than verify method, that could be revised). I think this would allow simpler error messages/more control, simpler code and earlier flagging of issues. It would still be possible to add a static assert manually if not using generated code.

Could you try that and see what it looks like?

Refine the template structure and added test. Will add doc for this.
Other parts can start the review.

Chia-hungDuan retitled this revision from [mlir] Support verification order (1/3) [WIP] to [mlir] Support verification order (1/3).Dec 13 2021, 11:07 PM
Chia-hungDuan added a comment.EditedDec 13 2021, 11:12 PM

@jpienaar , Sorry I didn't notice your comment until uploaded a new revision. Will address them later.

Check the dependency from tblgen instead.

jpienaar accepted this revision.Jan 24 2022, 7:04 AM

Nice, thanks for looking at the alternative specification (I think this is much simpler with nice error messages early in build process)

mlir/lib/TableGen/Operator.cpp
564

declared before could be overloaded here (the definition of the trait itself in the file vs the order listed on the op). Don't have too good a suggestion here for working. But let's add a comment with an example here: if this failure is hit and folks search in the code they can see an example.

(Perhaps the wording of the error could be "trait X requires trait Y to precede it in traits list")

This revision is now accepted and ready to land.Jan 24 2022, 7:04 AM

Rebase and address the review comments

mlir/lib/TableGen/Operator.cpp
564

Done.

This revision was landed with ongoing or failed builds.Feb 2 2022, 10:27 AM
This revision was automatically updated to reflect the committed changes.