This is an archive of the discontinued LLVM Phabricator instance.

[mlir][irdl] Add IRDL verification constraint classes
ClosedPublic

Authored by math-fehr on Mar 9 2023, 3:18 PM.

Details

Summary

This patch adds the necessary constraint classes that are be used
by IRDL to define Operation, Type, and Attribute verifiers.

A constraint is a class inheriting the irdl::Constraint class,
which may call other constraints that are indexed by unsigned.
A constraint represent an invariant over an Attribute.

The ConstraintVerifier class group these constraints together,
and make sure that a constraint can only identify a single
attribute. So, once a constraint is used to check the
satisfiability of an Attribute, the Attribute will be
memorized for this constraint. This ensure that in IRDL, a
single !irdl.attribute value only correspond to a single
Attribute.

Depends on D144693

Diff Detail

Event Timeline

math-fehr created this revision.Mar 9 2023, 3:18 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 9 2023, 3:18 PM
math-fehr requested review of this revision.Mar 9 2023, 3:18 PM

Are there tests that can be written for this?

All of these are tested in D145734 with filecheck. Do you think I should still add additional unit tests for these?

ping @rriddle, just to know if I should write unit tests or if the filecheck tests in D145734 are sufficient.

math-fehr updated this revision to Diff 516438.Apr 24 2023, 9:19 AM

Improve includes, and remove unneeded namespace prefixes

Mogball accepted this revision.Apr 26 2023, 8:34 AM
Mogball added inline comments.
mlir/include/mlir/Dialect/IRDL/IRDLVerifiers.h
57

Do you need the full tri state?

89

Do you need this vtable hook?

108

Same for all of these

mlir/lib/Dialect/IRDL/IRDLVerifiers.cpp
36

Please add braces here

This revision is now accepted and ready to land.Apr 26 2023, 8:34 AM
math-fehr updated this revision to Diff 517355.Apr 26 2023, 3:18 PM
math-fehr marked 3 inline comments as done.

Address comments

For the tri-state problem, I think that we want to let users use empty Attribute in their Attribute parameters.
For instance, if you would define a vector in IRDL, you may use the null attribute to represent the vector encoding.

Do you think that this is sensible, or that we should refrain users from using a null attribute in attribute parameters?

mlir/include/mlir/Dialect/IRDL/IRDLVerifiers.h
57

I added a comment to actually reflect that (compared to the previously bad comment).
We need that state because some parameters might use null attributes to encode parameters.
For instance, if you would define a vector in IRDL, you may use the null attribute to represent the vector encoding.
Do you think that this is sensible, or that we should refrain users from using a null attribute in attribute parameters?

89

As far as I know yes, and I get a warning if I don't add them: 'mlir::irdl::Constraint' has virtual functions but non-virtual destructor.

math-fehr updated this revision to Diff 519790.May 5 2023, 3:14 AM

Rebase to main

This revision was landed with ongoing or failed builds.May 5 2023, 3:17 AM
This revision was automatically updated to reflect the committed changes.