Skip to content

Commit 3238de5

Browse files
krzysz00krzysz00
committed
Review comments
1 parent 987a7d0 commit 3238de5

File tree

6 files changed

+70
-47
lines changed

6 files changed

+70
-47
lines changed

‎mlir/include/mlir/IR/Constraints.td

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,12 @@ class Or<list<Pred> children> : CombinedPred<PredCombinerOr, children>;
9393
// A predicate that holds if its child does not.
9494
class Neg<Pred child> : CombinedPred<PredCombinerNot, [child]>;
9595

96+
// A predicate that is always true.
97+
defvar TruePred = And<[]>;
98+
99+
// A predicate that is always false.
100+
defvar False = Or<[]>;
101+
96102
// A predicate that substitutes "pat" with "repl" in predicate calls of the
97103
// leaves of the predicate tree (i.e., not CombinedPred).
98104
//

‎mlir/include/mlir/IR/Properties.td

Lines changed: 22 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -65,11 +65,12 @@ class Property<string storageTypeParam = "", string desc = ""> {
6565
return convertFromAttribute($_storage, $_attr, $_diag);
6666
}];
6767

68-
// The verification predicate for this property. Defaults to And<[]>,
69-
// which is trivially true, since properties are always their expected type.
68+
// The verification predicate for this property. Defaults to the true predicate,
69+
// since properties are always their expected type.
7070
// Within the predicate, `$_self` is an instance of the **interface**
71-
// type of the property.
72-
Pred predicate = ?;
71+
// type of the property. Setting this field to ? will also result in a
72+
// true predicate but is not recommended, as it breaks composability.
73+
Pred predicate = TruePred;
7374

7475
// The call expression to hash the property.
7576
//
@@ -365,9 +366,12 @@ class DefaultValuedProperty<Property p, string default = "", string storageDefau
365366
let writeToMlirBytecode = p.writeToMlirBytecode;
366367
}
367368

369+
/// Apply the predicate `pred` to the property `p`, ANDing it with any
370+
/// predicates it may already have. If `newSummary` is provided, replace the
371+
/// summary of `p` with `newSummary`.
368372
class ConfinedProperty<Property p, Pred pred, string newSummary = "">
369373
: Property<p.storageType, !if(!empty(newSummary), p.summary, newSummary)> {
370-
let predicate = !if(!initialized(p.predicate), And<[p.predicate, pred]>, pred);
374+
let predicate = !if(!ne(p.predicate, TruePred), And<[p.predicate, pred]>, pred);
371375
let baseProperty = p;
372376
// Keep up to date with `Property` above.
373377
let description = p.description;
@@ -409,17 +413,15 @@ class _makePropStorage<Property prop, string name> {
409413
/// }
410414
///
411415
/// and then appends `prefix` and `suffix`.
412-
class _makeChildWrapperPred<string prefix, Property wrappedProp, string suffix> {
416+
class _makeStoragedWrapperPred<Property wrappedProp> {
413417
Pred ret =
414-
!if(!initialized(wrappedProp.predicate),
415-
Concat<
416-
prefix # "[]("
417-
# "const " # wrappedProp.storageType # "& baseStore) -> bool { return []("
418-
# wrappedProp.interfaceType # " baseIface) -> bool { return (",
419-
SubstLeaves<"$_self", "baseIface", wrappedProp.predicate>,
420-
"); }(" # !subst("$_storage", "baseStore", wrappedProp.convertFromStorage)
421-
# "); }" # suffix
422-
>, ?);
418+
Concat<
419+
"[](" # "const " # wrappedProp.storageType
420+
# "& baseStore) -> bool { return []("
421+
# wrappedProp.interfaceType # " baseIface) -> bool { return (",
422+
SubstLeaves<"$_self", "baseIface", wrappedProp.predicate>,
423+
"); }(" # !subst("$_storage", "baseStore", wrappedProp.convertFromStorage)
424+
# "); }">;
423425
}
424426

425427
/// The generic class for arrays of some other property, which is stored as a
@@ -462,7 +464,9 @@ class ArrayProperty<Property elem = Property<>, string newSummary = ""> :
462464
return ::mlir::ArrayAttr::get($_ctxt, elems);
463465
}];
464466

465-
let predicate = _makeChildWrapperPred<"::llvm::all_of($_self, ", elem, ")">.ret;
467+
let predicate = !if(!eq(elem.predicate, TruePred),
468+
TruePred,
469+
Concat<"::llvm::all_of($_self, ", _makeStoragedWrapperPred<elem>.ret, ")">);
466470

467471
defvar theParserBegin = [{
468472
auto& storage = $_storage;
@@ -633,10 +637,10 @@ class OptionalProperty<Property p, bit canDelegateParsing = 1>
633637
return ::mlir::ArrayAttr::get($_ctxt, {attr});
634638
}];
635639

636-
let predicate = !if(!initialized(p.predicate),
640+
let predicate = !if(!ne(p.predicate, TruePred),
637641
Or<[CPred<"!$_self.has_value()">,
638642
SubstLeaves<"$_self", "(*($_self))", p.predicate>]>,
639-
?);
643+
TruePred);
640644

641645
defvar delegatedParserBegin = [{
642646
if (::mlir::succeeded($_parser.parseOptionalKeyword("none"))) {

‎mlir/lib/TableGen/Predicate.cpp

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -235,6 +235,16 @@ propagateGroundTruth(PredNode *node,
235235
return node;
236236
}
237237

238+
if (node->kind == PredCombinerKind::And && node->children.empty()) {
239+
node->kind = PredCombinerKind::True;
240+
return node;
241+
}
242+
243+
if (node->kind == PredCombinerKind::Or && node->children.empty()) {
244+
node->kind = PredCombinerKind::False;
245+
return node;
246+
}
247+
238248
// Otherwise, look at child nodes.
239249

240250
// Move child nodes into some local variable so that they can be optimized

‎mlir/test/IR/test-op-property-predicates.mlir

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ test.op_with_property_predicates <{
2323

2424
// -----
2525

26-
// expected-error @+1 {{'test.op_with_property_predicates' op property 'scalar' failed to satiisfy constraint: non-negative int64_t}}
26+
// expected-error @+1 {{'test.op_with_property_predicates' op property 'scalar' failed to satisfy constraint: non-negative int64_t}}
2727
test.op_with_property_predicates <{
2828
scalar = -1 : i64,
2929
optional = [2 : i64],
@@ -37,7 +37,7 @@ test.op_with_property_predicates <{
3737

3838
// -----
3939

40-
// expected-error @+1 {{'test.op_with_property_predicates' op property 'optional' failed to satiisfy constraint: optional non-negative int64_t}}
40+
// expected-error @+1 {{'test.op_with_property_predicates' op property 'optional' failed to satisfy constraint: optional non-negative int64_t}}
4141
test.op_with_property_predicates <{
4242
scalar = 1 : i64,
4343
optional = [-1 : i64],
@@ -51,7 +51,7 @@ test.op_with_property_predicates <{
5151

5252
// -----
5353

54-
// expected-error @+1 {{'test.op_with_property_predicates' op property 'defaulted' failed to satiisfy constraint: non-negative int64_t}}
54+
// expected-error @+1 {{'test.op_with_property_predicates' op property 'defaulted' failed to satisfy constraint: non-negative int64_t}}
5555
test.op_with_property_predicates <{
5656
scalar = 1 : i64,
5757
optional = [2 : i64],
@@ -65,7 +65,7 @@ test.op_with_property_predicates <{
6565

6666
// -----
6767

68-
// expected-error @+1 {{'test.op_with_property_predicates' op property 'more_constrained' failed to satiisfy constraint: between 0 and 5}}
68+
// expected-error @+1 {{'test.op_with_property_predicates' op property 'more_constrained' failed to satisfy constraint: between 0 and 5}}
6969
test.op_with_property_predicates <{
7070
scalar = 1 : i64,
7171
optional = [2 : i64],
@@ -79,7 +79,7 @@ test.op_with_property_predicates <{
7979

8080
// -----
8181

82-
// expected-error @+1 {{'test.op_with_property_predicates' op property 'array' failed to satiisfy constraint: array of non-negative int64_t}}
82+
// expected-error @+1 {{'test.op_with_property_predicates' op property 'array' failed to satisfy constraint: array of non-negative int64_t}}
8383
test.op_with_property_predicates <{
8484
scalar = 1 : i64,
8585
optional = [2 : i64],
@@ -93,7 +93,7 @@ test.op_with_property_predicates <{
9393

9494
// -----
9595

96-
// expected-error @+1 {{'test.op_with_property_predicates' op property 'non_empty_unconstrained' failed to satiisfy constraint: non-empty array of int64_t}}
96+
// expected-error @+1 {{'test.op_with_property_predicates' op property 'non_empty_unconstrained' failed to satisfy constraint: non-empty array of int64_t}}
9797
test.op_with_property_predicates <{
9898
scalar = 1 : i64,
9999
optional = [2 : i64],
@@ -107,7 +107,7 @@ test.op_with_property_predicates <{
107107

108108
// -----
109109

110-
// expected-error @+1 {{'test.op_with_property_predicates' op property 'non_empty_constrained' failed to satiisfy constraint: non-empty array of non-negative int64_t}}
110+
// expected-error @+1 {{'test.op_with_property_predicates' op property 'non_empty_constrained' failed to satisfy constraint: non-empty array of non-negative int64_t}}
111111
test.op_with_property_predicates <{
112112
scalar = 1 : i64,
113113
optional = [2 : i64],
@@ -121,7 +121,7 @@ test.op_with_property_predicates <{
121121

122122
// -----
123123

124-
// expected-error @+1 {{'test.op_with_property_predicates' op property 'non_empty_constrained' failed to satiisfy constraint: non-empty array of non-negative int64_t}}
124+
// expected-error @+1 {{'test.op_with_property_predicates' op property 'non_empty_constrained' failed to satisfy constraint: non-empty array of non-negative int64_t}}
125125
test.op_with_property_predicates <{
126126
scalar = 1 : i64,
127127
optional = [2 : i64],
@@ -135,7 +135,7 @@ test.op_with_property_predicates <{
135135

136136
// -----
137137

138-
// expected-error @+1 {{'test.op_with_property_predicates' op property 'non_empty_optional' failed to satiisfy constraint: optional non-empty array of non-negative int64_t}}
138+
// expected-error @+1 {{'test.op_with_property_predicates' op property 'non_empty_optional' failed to satisfy constraint: optional non-empty array of non-negative int64_t}}
139139
test.op_with_property_predicates <{
140140
scalar = 1 : i64,
141141
optional = [2 : i64],

‎mlir/test/mlir-tblgen/op-properties-predicates.td

Lines changed: 17 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -36,37 +36,38 @@ def OpWithPredicates : NS_Op<"op_with_predicates"> {
3636
}
3737

3838
// CHECK-LABEL: OpWithPredicates::verifyInvariantsImpl()
39-
// CHECK: int64_t tblgen_scalar = this->getScalar(); (void)tblgen_scalar;
39+
// Note: for test readibility, we capture [[maybe_unused]] into the variable maybe_unused
40+
// CHECK: [[maybe_unused:\[\[maybe_unused\]\]]] int64_t tblgen_scalar = this->getScalar();
4041
// CHECK: if (!((tblgen_scalar >= 0)))
41-
// CHECK-NEXT: return emitOpError("property 'scalar' failed to satiisfy constraint: non-negative int64_t");
42+
// CHECK-NEXT: return emitOpError("property 'scalar' failed to satisfy constraint: non-negative int64_t");
4243

43-
// CHECK: std::optional<int64_t> tblgen_optional = this->getOptional(); (void)tblgen_optional;
44+
// CHECK: [[maybe_unused]] std::optional<int64_t> tblgen_optional = this->getOptional();
4445
// CHECK: if (!(((!tblgen_optional.has_value())) || (((*(tblgen_optional)) >= 0))))
45-
// CHECK-NEXT: return emitOpError("property 'optional' failed to satiisfy constraint: optional non-negative int64_t");
46+
// CHECK-NEXT: return emitOpError("property 'optional' failed to satisfy constraint: optional non-negative int64_t");
4647

47-
// CHECK: int64_t tblgen_defaulted = this->getDefaulted(); (void)tblgen_defaulted;
48+
// CHECK: [[maybe_unused]] int64_t tblgen_defaulted = this->getDefaulted();
4849
// CHECK: if (!((tblgen_defaulted >= 0)))
49-
// CHECK-NEXT: return emitOpError("property 'defaulted' failed to satiisfy constraint: non-negative int64_t");
50+
// CHECK-NEXT: return emitOpError("property 'defaulted' failed to satisfy constraint: non-negative int64_t");
5051

51-
// CHECK: int64_t tblgen_moreConstrained = this->getMoreConstrained(); (void)tblgen_moreConstrained;
52+
// CHECK: [[maybe_unused]] int64_t tblgen_moreConstrained = this->getMoreConstrained();
5253
// CHECK: if (!(((tblgen_moreConstrained >= 0)) && ((tblgen_moreConstrained <= 5))))
53-
// CHECK-NEXT: return emitOpError("property 'moreConstrained' failed to satiisfy constraint: between 0 and 5");
54+
// CHECK-NEXT: return emitOpError("property 'moreConstrained' failed to satisfy constraint: between 0 and 5");
5455

55-
// CHECK: ::llvm::ArrayRef<int64_t> tblgen_array = this->getArray(); (void)tblgen_array;
56+
// CHECK: [[maybe_unused]] ::llvm::ArrayRef<int64_t> tblgen_array = this->getArray();
5657
// CHECK: if (!(::llvm::all_of(tblgen_array, [](const int64_t& baseStore) -> bool { return [](int64_t baseIface) -> bool { return ((baseIface >= 0)); }(baseStore); })))
57-
// CHECK-NEXT: return emitOpError("property 'array' failed to satiisfy constraint: array of non-negative int64_t");
58+
// CHECK-NEXT: return emitOpError("property 'array' failed to satisfy constraint: array of non-negative int64_t");
5859

59-
// CHECK: ::llvm::ArrayRef<int64_t> tblgen_nonEmptyUnconstrained = this->getNonEmptyUnconstrained(); (void)tblgen_nonEmptyUnconstrained;
60+
// CHECK: [[maybe_unused]] ::llvm::ArrayRef<int64_t> tblgen_nonEmptyUnconstrained = this->getNonEmptyUnconstrained();
6061
// CHECK: if (!(!((tblgen_nonEmptyUnconstrained.empty()))))
61-
// CHECK-NEXT: return emitOpError("property 'non_empty_unconstrained' failed to satiisfy constraint: non-empty array of int64_t");
62+
// CHECK-NEXT: return emitOpError("property 'non_empty_unconstrained' failed to satisfy constraint: non-empty array of int64_t");
6263

63-
// CHECK: ::llvm::ArrayRef<int64_t> tblgen_nonEmptyConstrained = this->getNonEmptyConstrained(); (void)tblgen_nonEmptyConstrained;
64+
// CHECK: [[maybe_unused]] ::llvm::ArrayRef<int64_t> tblgen_nonEmptyConstrained = this->getNonEmptyConstrained();
6465
// CHECK: if (!((::llvm::all_of(tblgen_nonEmptyConstrained, [](const int64_t& baseStore) -> bool { return [](int64_t baseIface) -> bool { return ((baseIface >= 0)); }(baseStore); })) && (!((tblgen_nonEmptyConstrained.empty())))))
65-
// CHECK-NEXT: return emitOpError("property 'non_empty_constrained' failed to satiisfy constraint: non-empty array of non-negative int64_t");
66+
// CHECK-NEXT: return emitOpError("property 'non_empty_constrained' failed to satisfy constraint: non-empty array of non-negative int64_t");
6667

67-
// CHECK: std::optional<::llvm::ArrayRef<int64_t>> tblgen_nonEmptyOptional = this->getNonEmptyOptional(); (void)tblgen_nonEmptyOptional;
68+
// CHECK: [[maybe_unused]] std::optional<::llvm::ArrayRef<int64_t>> tblgen_nonEmptyOptional = this->getNonEmptyOptional();
6869
// CHECK: (!(((!tblgen_nonEmptyOptional.has_value())) || ((::llvm::all_of((*(tblgen_nonEmptyOptional)), [](const int64_t& baseStore) -> bool { return [](int64_t baseIface) -> bool { return ((baseIface >= 0)); }(baseStore); })) && (!(((*(tblgen_nonEmptyOptional)).empty()))))))
69-
// CHECK-NEXT: return emitOpError("property 'non_empty_optional' failed to satiisfy constraint: optional non-empty array of non-negative int64_t");
70+
// CHECK-NEXT: return emitOpError("property 'non_empty_optional' failed to satisfy constraint: optional non-empty array of non-negative int64_t");
7071

7172
// CHECK-NOT: int64_t tblgen_unconstrained
7273
// CHECK: return ::mlir::success();

‎mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1042,18 +1042,18 @@ static void genPropertyVerifier(const OpOrAdaptorHelper &emitHelper,
10421042
// {1}: Property name, with the first letter capitalized, to find the getter.
10431043
// {2}: Property interface type.
10441044
const char *const fetchProperty = R"(
1045-
{2} {0} = this->get{1}(); (void){0};
1045+
[[maybe_unused]] {2} {0} = this->get{1}();
10461046
)";
10471047

10481048
// Code to verify that the predicate of a property holds. Embeds the
10491049
// condition inline.
1050-
// {0}: Property condition code, pre-tgfmt()'d.
1050+
// {0}: Property condition code, with tgfmt() applied.
10511051
// {1}: Emit error prefix.
10521052
// {2}: Property name.
10531053
// {3}: Property description.
10541054
const char *const verifyProperty = R"(
10551055
if (!({0}))
1056-
return {1}"property '{2}' failed to satiisfy constraint: {3}");
1056+
return {1}"property '{2}' failed to satisfy constraint: {3}");
10571057
)";
10581058

10591059
// Prefix variables with `tblgen_` to avoid hiding the attribute accessor.
@@ -1063,18 +1063,20 @@ static void genPropertyVerifier(const OpOrAdaptorHelper &emitHelper,
10631063
return (tblgenNamePrefix + Twine(varName)).str();
10641064
};
10651065

1066-
body.indent();
10671066
for (const NamedProperty &prop : emitHelper.getOp().getProperties()) {
10681067
Pred predicate = prop.prop.getPredicate();
10691068
// Null predicate, nothing to verify.
10701069
if (predicate == Pred())
10711070
continue;
10721071

10731072
std::string rawCondition = predicate.getCondition();
1073+
if (rawCondition == "true")
1074+
continue;
10741075
bool needsOp = StringRef(rawCondition).contains("$_op");
10751076
if (needsOp && !emitHelper.isEmittingForOp())
10761077
continue;
10771078

1079+
auto scope = body.scope("{\n", "}\n", /*indent=*/true);
10781080
std::string varName = getVarName(prop);
10791081
std::string getterName =
10801082
convertToCamelFromSnakeCase(prop.name, /*capitalizeFirst=*/true);

0 commit comments

Comments
 (0)