[Mlir-commits] [mlir] 9613a85 - [mlir:PDL] Rework errors for pdl.operations with non-inferrable results
River Riddle
llvmlistbot at llvm.org
Thu Apr 28 12:58:44 PDT 2022
Author: River Riddle
Date: 2022-04-28T12:58:00-07:00
New Revision: 9613a850b69d2abf621d4c4a9dd9d967c580bd80
URL: https://github.com/llvm/llvm-project/commit/9613a850b69d2abf621d4c4a9dd9d967c580bd80
DIFF: https://github.com/llvm/llvm-project/commit/9613a850b69d2abf621d4c4a9dd9d967c580bd80.diff
LOG: [mlir:PDL] Rework errors for pdl.operations with non-inferrable results
We currently emit an error during verification if a pdl.operation with non-inferrable
results is used within a rewrite. This allows for catching some errors during compile
time, but is slightly broken. For one, the verification at the PDL level assumes that
all dialects have been loaded, which is true at run time, but may not be true when
the PDL is generated (such as via PDLL). This commit fixes this by not emitting the
error if the operation isn't registered, i.e. it uses the `mightHave` variant of trait/interface
methods.
Secondly, we currently don't verify when a pdl.operation has no explicit results, but the
operation being created is known to expect at least one. This commit adds a heuristic
error to detect these cases when possible and fail. We can't always capture when the user
made an error, but we can capture the most common case where the user expected an
operation to infer its result types (when it actually isn't possible).
Differential Revision: https://reviews.llvm.org/D124583
Added:
Modified:
mlir/include/mlir/Dialect/PDL/IR/PDLOps.td
mlir/include/mlir/IR/OperationSupport.h
mlir/lib/Dialect/PDL/IR/PDL.cpp
mlir/test/Dialect/PDL/invalid.mlir
Removed:
################################################################################
diff --git a/mlir/include/mlir/Dialect/PDL/IR/PDLOps.td b/mlir/include/mlir/Dialect/PDL/IR/PDLOps.td
index dfd8318b80885..1a4f7211696f3 100644
--- a/mlir/include/mlir/Dialect/PDL/IR/PDLOps.td
+++ b/mlir/include/mlir/Dialect/PDL/IR/PDLOps.td
@@ -369,6 +369,12 @@ def PDL_OperationOp : PDL_Op<"operation", [AttrSizedOperandSegments]> {
/// Returns true if the operation type referenced supports result type
/// inference.
bool hasTypeInference();
+
+ /// Returns true if the operation type referenced might support result type
+ /// inference, i.e. it supports type reference or is currently not
+ /// registered in the context. Returns false if the root operation name
+ /// has not been set.
+ bool mightHaveTypeInference();
}];
let hasVerifier = 1;
}
diff --git a/mlir/include/mlir/IR/OperationSupport.h b/mlir/include/mlir/IR/OperationSupport.h
index d995518282eee..de09ca58a4092 100644
--- a/mlir/include/mlir/IR/OperationSupport.h
+++ b/mlir/include/mlir/IR/OperationSupport.h
@@ -167,6 +167,17 @@ class OperationName {
return impl->interfaceMap.contains(interfaceID);
}
+ /// Returns true if the operation *might* have the provided interface. This
+ /// means that either the operation is unregistered, or it was registered with
+ /// the provide interface.
+ template <typename T>
+ bool mightHaveInterface() const {
+ return mightHaveInterface(TypeID::get<T>());
+ }
+ bool mightHaveInterface(TypeID interfaceID) const {
+ return !isRegistered() || hasInterface(interfaceID);
+ }
+
/// Return the dialect this operation is registered to if the dialect is
/// loaded in the context, or nullptr if the dialect isn't loaded.
Dialect *getDialect() const {
diff --git a/mlir/lib/Dialect/PDL/IR/PDL.cpp b/mlir/lib/Dialect/PDL/IR/PDL.cpp
index 1d22cf5395b1d..417a2da11fc87 100644
--- a/mlir/lib/Dialect/PDL/IR/PDL.cpp
+++ b/mlir/lib/Dialect/PDL/IR/PDL.cpp
@@ -198,6 +198,36 @@ static LogicalResult verifyResultTypesAreInferrable(OperationOp op,
if (llvm::any_of(op.op().getUses(), canInferTypeFromUse))
return success();
+ // Handle the case where the operation has no explicit result types.
+ if (resultTypes.empty()) {
+ // If we don't know the concrete operation, don't attempt any verification.
+ // We can't make assumptions if we don't know the concrete operation.
+ Optional<StringRef> rawOpName = op.name();
+ if (!rawOpName)
+ return success();
+ Optional<RegisteredOperationName> opName =
+ RegisteredOperationName::lookup(*rawOpName, op.getContext());
+ if (!opName)
+ return success();
+
+ // If no explicit result types were provided, check to see if the operation
+ // expected at least one result. This doesn't cover all cases, but this
+ // should cover many cases in which the user intended to infer the results
+ // of an operation, but it isn't actually possible.
+ bool expectedAtLeastOneResult =
+ !opName->hasTrait<OpTrait::ZeroResult>() &&
+ !opName->hasTrait<OpTrait::VariadicResults>();
+ if (expectedAtLeastOneResult) {
+ return op
+ .emitOpError("must have inferable or constrained result types when "
+ "nested within `pdl.rewrite`")
+ .attachNote()
+ .append("operation is created in a non-inferrable context, but '",
+ *opName, "' does not implement InferTypeOpInterface");
+ }
+ return success();
+ }
+
// Otherwise, make sure each of the types can be inferred.
for (const auto &it : llvm::enumerate(resultTypes)) {
Operation *resultTypeOp = it.value().getDefiningOp();
@@ -248,7 +278,7 @@ LogicalResult OperationOp::verify() {
// If the operation is within a rewrite body and doesn't have type inference,
// ensure that the result types can be resolved.
- if (isWithinRewrite && !hasTypeInference()) {
+ if (isWithinRewrite && !mightHaveTypeInference()) {
if (failed(verifyResultTypesAreInferrable(*this, types())))
return failure();
}
@@ -257,12 +287,18 @@ LogicalResult OperationOp::verify() {
}
bool OperationOp::hasTypeInference() {
- Optional<StringRef> opName = name();
- if (!opName)
- return false;
+ if (Optional<StringRef> rawOpName = name()) {
+ OperationName opName(*rawOpName, getContext());
+ return opName.hasInterface<InferTypeOpInterface>();
+ }
+ return false;
+}
- if (auto rInfo = RegisteredOperationName::lookup(*opName, getContext()))
- return rInfo->hasInterface<InferTypeOpInterface>();
+bool OperationOp::mightHaveTypeInference() {
+ if (Optional<StringRef> rawOpName = name()) {
+ OperationName opName(*rawOpName, getContext());
+ return opName.mightHaveInterface<InferTypeOpInterface>();
+ }
return false;
}
diff --git a/mlir/test/Dialect/PDL/invalid.mlir b/mlir/test/Dialect/PDL/invalid.mlir
index 09d4467dcd88e..954b5d0ab0251 100644
--- a/mlir/test/Dialect/PDL/invalid.mlir
+++ b/mlir/test/Dialect/PDL/invalid.mlir
@@ -136,7 +136,21 @@ pdl.pattern : benefit(1) {
// expected-error at below {{op must have inferable or constrained result types when nested within `pdl.rewrite`}}
// expected-note at below {{result type #0 was not constrained}}
- %newOp = operation "foo.op" -> (%type : !pdl.type)
+ %newOp = operation "builtin.unrealized_conversion_cast" -> (%type : !pdl.type)
+ }
+}
+
+// -----
+
+// Unused operation only necessary to ensure the func dialect is loaded.
+func.func private @unusedOpToLoadFuncDialect()
+
+pdl.pattern : benefit(1) {
+ %op = operation "foo.op"
+ rewrite %op {
+ // expected-error at below {{op must have inferable or constrained result types when nested within `pdl.rewrite`}}
+ // expected-note at below {{operation is created in a non-inferrable context, but 'func.constant' does not implement InferTypeOpInterface}}
+ %newOp = operation "func.constant"
}
}
More information about the Mlir-commits
mailing list