[Mlir-commits] [mlir] Simplify diagnostic error management for MLIR properties API (NFC) (PR #67409)
llvmlistbot at llvm.org
llvmlistbot at llvm.org
Tue Sep 26 02:27:07 PDT 2023
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-mlir
<details>
<summary>Changes</summary>
This is a follow-up to 8c2bff1ab929 which lazy-initialized the diagnostic and removed the need to dynamically abandon() an InFlightDiagnostic. This further simplifies the code to not needed to return a reference to an InFlightDiagnostic and instead eagerly emit errors.
Also use `emitError` as name instead of `getDiag` which seems more explicit and in-line with the common usage.
---
Patch is 33.16 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/67409.diff
18 Files Affected:
- (modified) mlir/include/mlir/IR/ExtensibleDialect.h (+3-3)
- (modified) mlir/include/mlir/IR/ODSSupport.h (+3-3)
- (modified) mlir/include/mlir/IR/OpDefinition.h (+2-2)
- (modified) mlir/include/mlir/IR/Operation.h (+1-1)
- (modified) mlir/include/mlir/IR/OperationSupport.h (+15-14)
- (modified) mlir/lib/AsmParser/Parser.cpp (+9-19)
- (modified) mlir/lib/CAPI/IR/IR.cpp (+5-10)
- (modified) mlir/lib/IR/MLIRContext.cpp (+2-2)
- (modified) mlir/lib/IR/ODSSupport.cpp (+10-10)
- (modified) mlir/lib/IR/Operation.cpp (+2-2)
- (modified) mlir/lib/IR/OperationSupport.cpp (+2-2)
- (modified) mlir/lib/TableGen/CodeGenHelpers.cpp (+2-2)
- (modified) mlir/test/lib/Dialect/Test/TestDialect.cpp (+12-12)
- (modified) mlir/test/lib/Dialect/Test/TestDialect.h (+1-1)
- (modified) mlir/test/mlir-tblgen/constraint-unique.td (+2-2)
- (modified) mlir/test/mlir-tblgen/interfaces-as-constraints.td (+2-2)
- (modified) mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp (+11-11)
- (modified) mlir/unittests/IR/OpPropertiesTest.cpp (+10-15)
``````````diff
diff --git a/mlir/include/mlir/IR/ExtensibleDialect.h b/mlir/include/mlir/IR/ExtensibleDialect.h
index baff6011ff84651..5ae1e9ab537af9d 100644
--- a/mlir/include/mlir/IR/ExtensibleDialect.h
+++ b/mlir/include/mlir/IR/ExtensibleDialect.h
@@ -476,7 +476,7 @@ class DynamicOpDefinition : public OperationName::Impl {
void populateInherentAttrs(Operation *op, NamedAttrList &attrs) final {}
LogicalResult
verifyInherentAttrs(OperationName opName, NamedAttrList &attributes,
- function_ref<InFlightDiagnostic()> getDiag) final {
+ function_ref<InFlightDiagnostic()> emitError) final {
return success();
}
int getOpPropertyByteSize() final { return 0; }
@@ -489,8 +489,8 @@ class DynamicOpDefinition : public OperationName::Impl {
LogicalResult
setPropertiesFromAttr(OperationName opName, OpaqueProperties properties,
Attribute attr,
- function_ref<InFlightDiagnostic &()> getDiag) final {
- getDiag() << "extensible Dialects don't support properties";
+ function_ref<InFlightDiagnostic()> emitError) final {
+ emitError() << "extensible Dialects don't support properties";
return failure();
}
Attribute getPropertiesAsAttr(Operation *op) final { return {}; }
diff --git a/mlir/include/mlir/IR/ODSSupport.h b/mlir/include/mlir/IR/ODSSupport.h
index 748bf52a55c557a..70e3f986431e266 100644
--- a/mlir/include/mlir/IR/ODSSupport.h
+++ b/mlir/include/mlir/IR/ODSSupport.h
@@ -28,7 +28,7 @@ namespace mlir {
/// error message is also emitted.
LogicalResult
convertFromAttribute(int64_t &storage, Attribute attr,
- function_ref<InFlightDiagnostic &()> getDiag);
+ function_ref<InFlightDiagnostic()> emitError);
/// Convert the provided int64_t to an IntegerAttr attribute.
Attribute convertToAttribute(MLIRContext *ctx, int64_t storage);
@@ -39,7 +39,7 @@ Attribute convertToAttribute(MLIRContext *ctx, int64_t storage);
/// the optional diagnostic is provided an error message is also emitted.
LogicalResult
convertFromAttribute(MutableArrayRef<int64_t> storage, Attribute attr,
- function_ref<InFlightDiagnostic &()> getDiag);
+ function_ref<InFlightDiagnostic()> emitError);
/// Convert a DenseI32ArrayAttr to the provided storage. It is expected that the
/// storage has the same size as the array. An error is returned if the
@@ -47,7 +47,7 @@ convertFromAttribute(MutableArrayRef<int64_t> storage, Attribute attr,
/// the optional diagnostic is provided an error message is also emitted.
LogicalResult
convertFromAttribute(MutableArrayRef<int32_t> storage, Attribute attr,
- function_ref<InFlightDiagnostic &()> getDiag);
+ function_ref<InFlightDiagnostic()> emitError);
/// Convert the provided ArrayRef<int64_t> to a DenseI64ArrayAttr attribute.
Attribute convertToAttribute(MLIRContext *ctx, ArrayRef<int64_t> storage);
diff --git a/mlir/include/mlir/IR/OpDefinition.h b/mlir/include/mlir/IR/OpDefinition.h
index 82d0e93a8ee2fa9..8ab37c1d51d6b6c 100644
--- a/mlir/include/mlir/IR/OpDefinition.h
+++ b/mlir/include/mlir/IR/OpDefinition.h
@@ -1741,8 +1741,8 @@ class Op : public OpState, public Traits<ConcreteType>... {
template <typename PropertiesTy>
static LogicalResult
setPropertiesFromAttr(PropertiesTy &prop, Attribute attr,
- function_ref<InFlightDiagnostic &()> getDiag) {
- return setPropertiesFromAttribute(prop, attr, getDiag);
+ function_ref<InFlightDiagnostic()> emitError) {
+ return setPropertiesFromAttribute(prop, attr, emitError);
}
/// Convert the provided properties to an attribute. This default
/// implementation forwards to a free function `getPropertiesAsAttribute` that
diff --git a/mlir/include/mlir/IR/Operation.h b/mlir/include/mlir/IR/Operation.h
index b815eaf8899d6fc..50283cd4aedbf35 100644
--- a/mlir/include/mlir/IR/Operation.h
+++ b/mlir/include/mlir/IR/Operation.h
@@ -884,7 +884,7 @@ class alignas(8) Operation final
/// generic format. An optional diagnostic can be passed in for richer errors.
LogicalResult
setPropertiesFromAttribute(Attribute attr,
- function_ref<InFlightDiagnostic &()> getDiag);
+ function_ref<InFlightDiagnostic()> emitError);
/// Copy properties from an existing other properties object. The two objects
/// must be the same type.
diff --git a/mlir/include/mlir/IR/OperationSupport.h b/mlir/include/mlir/IR/OperationSupport.h
index 9e9f89b95e6fd50..bb3d1643e1687ab 100644
--- a/mlir/include/mlir/IR/OperationSupport.h
+++ b/mlir/include/mlir/IR/OperationSupport.h
@@ -129,7 +129,7 @@ class OperationName {
virtual void populateInherentAttrs(Operation *op, NamedAttrList &attrs) = 0;
virtual LogicalResult
verifyInherentAttrs(OperationName opName, NamedAttrList &attributes,
- function_ref<InFlightDiagnostic()> getDiag) = 0;
+ function_ref<InFlightDiagnostic()> emitError) = 0;
virtual int getOpPropertyByteSize() = 0;
virtual void initProperties(OperationName opName, OpaqueProperties storage,
OpaqueProperties init) = 0;
@@ -138,7 +138,7 @@ class OperationName {
OpaqueProperties properties) = 0;
virtual LogicalResult
setPropertiesFromAttr(OperationName, OpaqueProperties, Attribute,
- function_ref<InFlightDiagnostic &()> getDiag) = 0;
+ function_ref<InFlightDiagnostic()> emitError) = 0;
virtual Attribute getPropertiesAsAttr(Operation *) = 0;
virtual void copyProperties(OpaqueProperties, OpaqueProperties) = 0;
virtual bool compareProperties(OpaqueProperties, OpaqueProperties) = 0;
@@ -210,7 +210,7 @@ class OperationName {
void populateInherentAttrs(Operation *op, NamedAttrList &attrs) final;
LogicalResult
verifyInherentAttrs(OperationName opName, NamedAttrList &attributes,
- function_ref<InFlightDiagnostic()> getDiag) final;
+ function_ref<InFlightDiagnostic()> emitError) final;
int getOpPropertyByteSize() final;
void initProperties(OperationName opName, OpaqueProperties storage,
OpaqueProperties init) final;
@@ -219,7 +219,7 @@ class OperationName {
OpaqueProperties properties) final;
LogicalResult
setPropertiesFromAttr(OperationName, OpaqueProperties, Attribute,
- function_ref<InFlightDiagnostic &()> getDiag) final;
+ function_ref<InFlightDiagnostic()> emitError) final;
Attribute getPropertiesAsAttr(Operation *) final;
void copyProperties(OpaqueProperties, OpaqueProperties) final;
bool compareProperties(OpaqueProperties, OpaqueProperties) final;
@@ -408,8 +408,8 @@ class OperationName {
/// attributes when parsed from the older generic syntax pre-Properties.
LogicalResult
verifyInherentAttrs(NamedAttrList &attributes,
- function_ref<InFlightDiagnostic()> getDiag) const {
- return getImpl()->verifyInherentAttrs(*this, attributes, getDiag);
+ function_ref<InFlightDiagnostic()> emitError) const {
+ return getImpl()->verifyInherentAttrs(*this, attributes, emitError);
}
/// This hooks return the number of bytes to allocate for the op properties.
int getOpPropertyByteSize() const {
@@ -439,8 +439,9 @@ class OperationName {
/// Define the op properties from the provided Attribute.
LogicalResult setOpPropertiesFromAttribute(
OperationName opName, OpaqueProperties properties, Attribute attr,
- function_ref<InFlightDiagnostic &()> getDiag) const {
- return getImpl()->setPropertiesFromAttr(opName, properties, attr, getDiag);
+ function_ref<InFlightDiagnostic()> emitError) const {
+ return getImpl()->setPropertiesFromAttr(opName, properties, attr,
+ emitError);
}
void copyOpProperties(OpaqueProperties lhs, OpaqueProperties rhs) const {
@@ -596,9 +597,9 @@ class RegisteredOperationName : public OperationName {
}
LogicalResult
verifyInherentAttrs(OperationName opName, NamedAttrList &attributes,
- function_ref<InFlightDiagnostic()> getDiag) final {
+ function_ref<InFlightDiagnostic()> emitError) final {
if constexpr (hasProperties)
- return ConcreteOp::verifyInherentAttrs(opName, attributes, getDiag);
+ return ConcreteOp::verifyInherentAttrs(opName, attributes, emitError);
return success();
}
// Detect if the concrete operation defined properties.
@@ -636,12 +637,12 @@ class RegisteredOperationName : public OperationName {
LogicalResult
setPropertiesFromAttr(OperationName opName, OpaqueProperties properties,
Attribute attr,
- function_ref<InFlightDiagnostic &()> getDiag) final {
+ function_ref<InFlightDiagnostic()> emitError) final {
if constexpr (hasProperties) {
auto p = properties.as<Properties *>();
- return ConcreteOp::setPropertiesFromAttr(*p, attr, getDiag);
+ return ConcreteOp::setPropertiesFromAttr(*p, attr, emitError);
}
- getDiag() << "this operation does not support properties";
+ emitError() << "this operation does not support properties";
return failure();
}
Attribute getPropertiesAsAttr(Operation *op) final {
@@ -1010,7 +1011,7 @@ struct OperationState {
// optionally emit diagnostics on error through the provided diagnostic.
LogicalResult
setProperties(Operation *op,
- function_ref<InFlightDiagnostic &()> getDiag) const;
+ function_ref<InFlightDiagnostic()> emitError) const;
void addOperands(ValueRange newOperands);
diff --git a/mlir/lib/AsmParser/Parser.cpp b/mlir/lib/AsmParser/Parser.cpp
index 84f44dba806df01..8a9b8593110efb2 100644
--- a/mlir/lib/AsmParser/Parser.cpp
+++ b/mlir/lib/AsmParser/Parser.cpp
@@ -1446,16 +1446,11 @@ Operation *OperationParser::parseGenericOperation() {
// Try setting the properties for the operation, using a diagnostic to print
// errors.
if (properties) {
- std::unique_ptr<InFlightDiagnostic> diagnostic;
- auto getDiag = [&]() -> InFlightDiagnostic & {
- if (!diagnostic) {
- diagnostic = std::make_unique<InFlightDiagnostic>(
- mlir::emitError(srcLocation, "invalid properties ")
- << properties << " for op " << name << ": ");
- }
- return *diagnostic;
+ auto emitError = [&]() {
+ return mlir::emitError(srcLocation, "invalid properties ")
+ << properties << " for op " << name << ": ";
};
- if (failed(op->setPropertiesFromAttribute(properties, getDiag)))
+ if (failed(op->setPropertiesFromAttribute(properties, emitError)))
return nullptr;
}
@@ -2009,17 +2004,12 @@ OperationParser::parseCustomOperation(ArrayRef<ResultRecord> resultIDs) {
// Try setting the properties for the operation.
if (properties) {
- std::unique_ptr<InFlightDiagnostic> diagnostic;
- auto getDiag = [&]() -> InFlightDiagnostic & {
- if (!diagnostic) {
- diagnostic = std::make_unique<InFlightDiagnostic>(
- mlir::emitError(srcLocation, "invalid properties ")
- << properties << " for op " << op->getName().getStringRef()
- << ": ");
- }
- return *diagnostic;
+ auto emitError = [&]() {
+ return mlir::emitError(srcLocation, "invalid properties ")
+ << properties << " for op " << op->getName().getStringRef()
+ << ": ";
};
- if (failed(op->setPropertiesFromAttribute(properties, getDiag)))
+ if (failed(op->setPropertiesFromAttribute(properties, emitError)))
return nullptr;
}
return op;
diff --git a/mlir/lib/CAPI/IR/IR.cpp b/mlir/lib/CAPI/IR/IR.cpp
index 7f5c2aaee67382b..1d2ff6668e820c4 100644
--- a/mlir/lib/CAPI/IR/IR.cpp
+++ b/mlir/lib/CAPI/IR/IR.cpp
@@ -416,18 +416,13 @@ static LogicalResult inferOperationTypes(OperationState &state) {
auto prop = std::make_unique<char[]>(info->getOpPropertyByteSize());
properties = OpaqueProperties(prop.get());
if (properties) {
- std::unique_ptr<InFlightDiagnostic> diagnostic;
- auto getDiag = [&]() -> InFlightDiagnostic & {
- if (!diagnostic) {
- diagnostic = std::make_unique<InFlightDiagnostic>(
- emitError(state.location)
- << " failed properties conversion while building "
- << state.name.getStringRef() << " with `" << attributes << "`: ");
- }
- return *diagnostic;
+ auto emitError = [&]() {
+ return mlir::emitError(state.location)
+ << " failed properties conversion while building "
+ << state.name.getStringRef() << " with `" << attributes << "`: ";
};
if (failed(info->setOpPropertiesFromAttribute(state.name, properties,
- attributes, getDiag)))
+ attributes, emitError)))
return failure();
}
if (succeeded(inferInterface->inferReturnTypes(
diff --git a/mlir/lib/IR/MLIRContext.cpp b/mlir/lib/IR/MLIRContext.cpp
index ed1029959639587..e5a397c9d65c1ab 100644
--- a/mlir/lib/IR/MLIRContext.cpp
+++ b/mlir/lib/IR/MLIRContext.cpp
@@ -834,7 +834,7 @@ void OperationName::UnregisteredOpModel::populateInherentAttrs(
Operation *op, NamedAttrList &attrs) {}
LogicalResult OperationName::UnregisteredOpModel::verifyInherentAttrs(
OperationName opName, NamedAttrList &attributes,
- function_ref<InFlightDiagnostic()> getDiag) {
+ function_ref<InFlightDiagnostic()> emitError) {
return success();
}
int OperationName::UnregisteredOpModel::getOpPropertyByteSize() {
@@ -852,7 +852,7 @@ void OperationName::UnregisteredOpModel::populateDefaultProperties(
OperationName opName, OpaqueProperties properties) {}
LogicalResult OperationName::UnregisteredOpModel::setPropertiesFromAttr(
OperationName opName, OpaqueProperties properties, Attribute attr,
- function_ref<InFlightDiagnostic &()> getDiag) {
+ function_ref<InFlightDiagnostic()> emitError) {
*properties.as<Attribute *>() = attr;
return success();
}
diff --git a/mlir/lib/IR/ODSSupport.cpp b/mlir/lib/IR/ODSSupport.cpp
index 0601430c4461651..6e968d62e61c7f4 100644
--- a/mlir/lib/IR/ODSSupport.cpp
+++ b/mlir/lib/IR/ODSSupport.cpp
@@ -20,10 +20,10 @@ using namespace mlir;
LogicalResult
mlir::convertFromAttribute(int64_t &storage, Attribute attr,
- function_ref<InFlightDiagnostic &()> getDiag) {
+ function_ref<InFlightDiagnostic()> emitError) {
auto valueAttr = dyn_cast<IntegerAttr>(attr);
if (!valueAttr) {
- getDiag() << "expected IntegerAttr for key `value`";
+ emitError() << "expected IntegerAttr for key `value`";
return failure();
}
storage = valueAttr.getValue().getSExtValue();
@@ -36,16 +36,16 @@ Attribute mlir::convertToAttribute(MLIRContext *ctx, int64_t storage) {
template <typename DenseArrayTy, typename T>
LogicalResult
convertDenseArrayFromAttr(MutableArrayRef<T> storage, Attribute attr,
- function_ref<InFlightDiagnostic &()> getDiag,
+ function_ref<InFlightDiagnostic()> emitError,
StringRef denseArrayTyStr) {
auto valueAttr = dyn_cast<DenseArrayTy>(attr);
if (!valueAttr) {
- getDiag() << "expected " << denseArrayTyStr << " for key `value`";
+ emitError() << "expected " << denseArrayTyStr << " for key `value`";
return failure();
}
if (valueAttr.size() != static_cast<int64_t>(storage.size())) {
- getDiag() << "size mismatch in attribute conversion: " << valueAttr.size()
- << " vs " << storage.size();
+ emitError() << "size mismatch in attribute conversion: " << valueAttr.size()
+ << " vs " << storage.size();
return failure();
}
llvm::copy(valueAttr.asArrayRef(), storage.begin());
@@ -53,14 +53,14 @@ convertDenseArrayFromAttr(MutableArrayRef<T> storage, Attribute attr,
}
LogicalResult
mlir::convertFromAttribute(MutableArrayRef<int64_t> storage, Attribute attr,
- function_ref<InFlightDiagnostic &()> getDiag) {
- return convertDenseArrayFromAttr<DenseI64ArrayAttr>(storage, attr, getDiag,
+ function_ref<InFlightDiagnostic()> emitError) {
+ return convertDenseArrayFromAttr<DenseI64ArrayAttr>(storage, attr, emitError,
"DenseI64ArrayAttr");
}
LogicalResult
mlir::convertFromAttribute(MutableArrayRef<int32_t> storage, Attribute attr,
- function_ref<InFlightDiagnostic &()> getDiag) {
- return convertDenseArrayFromAttr<DenseI32ArrayAttr>(storage, attr, getDiag,
+ function_ref<InFlightDiagnostic()> emitError) {
+ return convertDenseArrayFromAttr<DenseI32ArrayAttr>(storage, attr, emitError,
"DenseI32ArrayAttr");
}
diff --git a/mlir/lib/IR/Operation.cpp b/mlir/lib/IR/Operation.cpp
index aa577aa089c6860..7507e0cd0c99b31 100644
--- a/mlir/lib/IR/Operation.cpp
+++ b/mlir/lib/IR/Operation.cpp
@@ -352,14 +352,14 @@ Attribute Operation::getPropertiesAsAttribute() {
return info->getOpPropertiesAsAttribute(this);
}
LogicalResult Operation::setPropertiesFromAttribute(
- Attribute attr, function_ref<InFlightDiagnostic &()> getDiag) {
+ Attribute attr, function_ref<InFlightDiagnostic()> emitError) {
std::optional<RegisteredOperationName> info = getRegisteredInfo();
if (LLVM_UNLIKELY(!info)) {
*getPropertiesStorage().as<Attribute *>() = attr;
return success();
}
return info->setOpPropertiesFromAttribute(
- this->getName(), this->getPropertiesStorage(), attr, getDiag);
+ this->getName(), this->getPropertiesStorage(), attr, emitError);
}
void Operation::copyProperties(OpaqueProperties rhs) {
diff --git a/mlir/lib/IR/OperationSupport.cpp b/mlir/lib/IR/OperationSupport.cpp
index b0c50f3d6e29855..5aac72fcb062c39 100644
--- a/mlir/lib/IR/OperationSupport.cpp
+++ b/mlir/lib/IR/OperationSupport.cpp
@@ -199,10 +199,10 @@ OperationState::~OperationState() {
}
LogicalResult OperationState::setProperties(
- Operation *op, function_ref<InFlightDiagnostic &()> getDiag) const {
+ Operation *op, function_ref<InFlightDiagnostic()> emitError) const {
if (LLVM_UNLIKELY(propertiesAttr)) {
assert(!properties);
- return op->setPropertiesFromAttribute(propertiesAttr, getDiag);
+ return op->setPropertiesFromAttribute(propertiesAttr, emitError);
}
if (properties)
propertiesSetter(op->getPropertiesStorage(), properties);
diff --git a/mlir/lib/TableGen/CodeGenHelpers.cpp b/mlir/lib/TableGen/CodeGenHelpers.cpp
index afd02b1a64ac942..371da8272029a00 100644
--- a/mlir/lib/TableGen/CodeGenHelpers.cpp
+++ b/mlir/lib/TableGen/CodeGenHelpers.cpp
@@ -133,9 +133,9 @@ static ::mlir::LogicalResult {0}(
/// functions are stripped anyways.
static const char *const attrConstraintCode = R"(
static ::mlir::LogicalResult {0}(
- ::mlir::Attribute attr, ::llvm::StringRef attrName, llvm::function_ref<::mlir::InFlightDiagnostic()> getDiag) {{
+ ::mlir::Attribute attr, ::llvm::StringRef attrName, llvm::function_ref<::mlir::InFlightDiagnostic()> emitError) {{
if (attr && !({1}))
- return getDiag() << "attribute '" << attrName
+ return emitError() << "attribute '" << attrName
<< "' failed to satisfy constraint: {2}";
return ::mlir::success();
}
diff --git a/mlir/test/lib/Dialect/Test/TestDialect.cpp b/mlir/test/lib/Dialect/Test/TestDialect.cpp
index 00c251936655d71..3f1bb667a0aecb5 100644
--- a/mlir/test/lib/Dialect/Test/TestDialect.cpp
+++ b/mlir/test/lib/Dialect/Test/TestDialect.cpp
@@ -55,10 +55,10 @@ Attribute MyPropStruct::asAttribute(MLIRContext *ctx) const {
}
LogicalResult
MyPropStruct::setFromAttr(MyPropStruct &prop, Attribute attr,
- function_ref<InFlightDiagnostic &()> getDiag) {
+ function_r...
[truncated]
``````````
</details>
https://github.com/llvm/llvm-project/pull/67409
More information about the Mlir-commits
mailing list