Lazy initialize diagnostic when handling MLIR properties #65868
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Instead of eagerly creating a diagnostic that will be discarded in the normal case, switch to lazy initialization on error.
joker-eph
changed the title
|
@llvm/pr-subscribers-mlir ChangesInstead of eagerly creating a diagnostic that will be discarded in the normal case, switch to lazy initialization on error.Patch is 31.66 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/65868.diff 16 Files Affected:
diff --git a/mlir/include/mlir/IR/ExtensibleDialect.h b/mlir/include/mlir/IR/ExtensibleDialect.h
index e4d8d2d6000fc60..37821d3a2a5163f 100644
--- a/mlir/include/mlir/IR/ExtensibleDialect.h
+++ b/mlir/include/mlir/IR/ExtensibleDialect.h
@@ -486,10 +486,11 @@ class DynamicOpDefinition : public OperationName::Impl {
void populateDefaultProperties(OperationName opName,
OpaqueProperties properties) final {}
- LogicalResult setPropertiesFromAttr(OperationName opName,
- OpaqueProperties properties,
- Attribute attr,
- InFlightDiagnostic *diag) final {
+ LogicalResult
+ setPropertiesFromAttr(OperationName opName, OpaqueProperties properties,
+ Attribute attr,
+ function_ref getDiag) final {
+ getDiag() << "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 687f764ae95fd99..748bf52a55c557a 100644
--- a/mlir/include/mlir/IR/ODSSupport.h
+++ b/mlir/include/mlir/IR/ODSSupport.h
@@ -14,6 +14,8 @@
#define MLIR_IR_ODSSUPPORT_H
#include "mlir/IR/Attributes.h"
+#include "mlir/IR/MLIRContext.h"
+#include "mlir/Support/LLVM.h"
namespace mlir {
@@ -24,8 +26,9 @@ namespace mlir {
/// Convert an IntegerAttr attribute to an int64_t, or return an error if the
/// attribute isn't an IntegerAttr. If the optional diagnostic is provided an
/// error message is also emitted.
-LogicalResult convertFromAttribute(int64_t &storage, Attribute attr,
- InFlightDiagnostic *diag);
+LogicalResult
+convertFromAttribute(int64_t &storage, Attribute attr,
+ function_ref getDiag);
/// Convert the provided int64_t to an IntegerAttr attribute.
Attribute convertToAttribute(MLIRContext *ctx, int64_t storage);
@@ -34,15 +37,17 @@ Attribute convertToAttribute(MLIRContext *ctx, int64_t storage);
/// storage has the same size as the array. An error is returned if the
/// attribute isn't a DenseI64ArrayAttr or it does not have the same size. If
/// the optional diagnostic is provided an error message is also emitted.
-LogicalResult convertFromAttribute(MutableArrayRef storage,
- Attribute attr, InFlightDiagnostic *diag);
+LogicalResult
+convertFromAttribute(MutableArrayRef storage, Attribute attr,
+ function_ref getDiag);
/// 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
/// attribute isn't a DenseI32ArrayAttr or it does not have the same size. If
/// the optional diagnostic is provided an error message is also emitted.
-LogicalResult convertFromAttribute(MutableArrayRef storage,
- Attribute attr, InFlightDiagnostic *diag);
+LogicalResult
+convertFromAttribute(MutableArrayRef storage, Attribute attr,
+ function_ref getDiag);
/// Convert the provided ArrayRef to a DenseI64ArrayAttr attribute.
Attribute convertToAttribute(MLIRContext *ctx, ArrayRef storage);
diff --git a/mlir/include/mlir/IR/OpDefinition.h b/mlir/include/mlir/IR/OpDefinition.h
index 84ba46f4d6f3ec1..895f17dfe1d07c8 100644
--- a/mlir/include/mlir/IR/OpDefinition.h
+++ b/mlir/include/mlir/IR/OpDefinition.h
@@ -1769,9 +1769,10 @@ class Op : public OpState, public Traits... {
/// the namespace where the properties are defined. It can also be overridden
/// in the derived ConcreteOp.
template
- static LogicalResult setPropertiesFromAttr(PropertiesTy &prop, Attribute attr,
- InFlightDiagnostic *diag) {
- return setPropertiesFromAttribute(prop, attr, diag);
+ static LogicalResult
+ setPropertiesFromAttr(PropertiesTy &prop, Attribute attr,
+ function_ref getDiag) {
+ return setPropertiesFromAttribute(prop, attr, getDiag);
}
/// 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 361a38e87b6ba32..b815eaf8899d6fc 100644
--- a/mlir/include/mlir/IR/Operation.h
+++ b/mlir/include/mlir/IR/Operation.h
@@ -882,8 +882,9 @@ class alignas(8) Operation final
/// matching the expectations of the properties for this operation. This is
/// mostly useful for unregistered operations or used when parsing the
/// generic format. An optional diagnostic can be passed in for richer errors.
- LogicalResult setPropertiesFromAttribute(Attribute attr,
- InFlightDiagnostic *diagnostic);
+ LogicalResult
+ setPropertiesFromAttribute(Attribute attr,
+ function_ref getDiag);
/// 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 670dd289c480a30..19ffddc30904897 100644
--- a/mlir/include/mlir/IR/OperationSupport.h
+++ b/mlir/include/mlir/IR/OperationSupport.h
@@ -136,9 +136,9 @@ class OperationName {
virtual void deleteProperties(OpaqueProperties) = 0;
virtual void populateDefaultProperties(OperationName opName,
OpaqueProperties properties) = 0;
- virtual LogicalResult setPropertiesFromAttr(OperationName, OpaqueProperties,
- Attribute,
- InFlightDiagnostic *) = 0;
+ virtual LogicalResult
+ setPropertiesFromAttr(OperationName, OpaqueProperties, Attribute,
+ function_ref getDiag) = 0;
virtual Attribute getPropertiesAsAttr(Operation *) = 0;
virtual void copyProperties(OpaqueProperties, OpaqueProperties) = 0;
virtual llvm::hash_code hashProperties(OpaqueProperties) = 0;
@@ -216,8 +216,9 @@ class OperationName {
void deleteProperties(OpaqueProperties) final;
void populateDefaultProperties(OperationName opName,
OpaqueProperties properties) final;
- LogicalResult setPropertiesFromAttr(OperationName, OpaqueProperties,
- Attribute, InFlightDiagnostic *) final;
+ LogicalResult
+ setPropertiesFromAttr(OperationName, OpaqueProperties, Attribute,
+ function_ref getDiag) final;
Attribute getPropertiesAsAttr(Operation *) final;
void copyProperties(OpaqueProperties, OpaqueProperties) final;
llvm::hash_code hashProperties(OpaqueProperties) final;
@@ -434,12 +435,10 @@ class OperationName {
}
/// Define the op properties from the provided Attribute.
- LogicalResult
- setOpPropertiesFromAttribute(OperationName opName,
- OpaqueProperties properties, Attribute attr,
- InFlightDiagnostic *diagnostic) const {
- return getImpl()->setPropertiesFromAttr(opName, properties, attr,
- diagnostic);
+ LogicalResult setOpPropertiesFromAttribute(
+ OperationName opName, OpaqueProperties properties, Attribute attr,
+ function_ref getDiag) const {
+ return getImpl()->setPropertiesFromAttr(opName, properties, attr, getDiag);
}
void copyOpProperties(OpaqueProperties lhs, OpaqueProperties rhs) const {
@@ -628,16 +627,15 @@ class RegisteredOperationName : public OperationName {
*properties.as());
}
- LogicalResult setPropertiesFromAttr(OperationName opName,
- OpaqueProperties properties,
- Attribute attr,
- InFlightDiagnostic *diag) final {
+ LogicalResult
+ setPropertiesFromAttr(OperationName opName, OpaqueProperties properties,
+ Attribute attr,
+ function_ref getDiag) final {
if constexpr (hasProperties) {
auto p = properties.as();
- return ConcreteOp::setPropertiesFromAttr(*p, attr, diag);
+ return ConcreteOp::setPropertiesFromAttr(*p, attr, getDiag);
}
- if (diag)
- *diag << "This operation does not support properties";
+ getDiag() << "this operation does not support properties";
return failure();
}
Attribute getPropertiesAsAttr(Operation *op) final {
@@ -997,8 +995,9 @@ struct OperationState {
// Set the properties defined on this OpState on the given operation,
// optionally emit diagnostics on error through the provided diagnostic.
- LogicalResult setProperties(Operation *op,
- InFlightDiagnostic *diagnostic) const;
+ LogicalResult
+ setProperties(Operation *op,
+ function_ref getDiag) const;
void addOperands(ValueRange newOperands);
diff --git a/mlir/include/mlir/IR/Properties.td b/mlir/include/mlir/IR/Properties.td
index 3d7de7e4f460e51..99da1763524fa94 100644
--- a/mlir/include/mlir/IR/Properties.td
+++ b/mlir/include/mlir/IR/Properties.td
@@ -55,7 +55,7 @@ class Property {
// Format:
// - `$_storage` is the storage type value.
// - `$_attr` is the attribute.
- // - `$_diag` is an optional Diagnostic pointer to emit error.
+ // - `$_diag` is a callback to get a Diagnostic to emit error.
//
// The expression must return a LogicalResult
code convertFromAttribute = [{
diff --git a/mlir/lib/AsmParser/Parser.cpp b/mlir/lib/AsmParser/Parser.cpp
index 02bf9a418063991..84f44dba806df01 100644
--- a/mlir/lib/AsmParser/Parser.cpp
+++ b/mlir/lib/AsmParser/Parser.cpp
@@ -18,8 +18,10 @@
#include "mlir/IR/AffineMap.h"
#include "mlir/IR/AsmState.h"
#include "mlir/IR/BuiltinOps.h"
+#include "mlir/IR/Diagnostics.h"
#include "mlir/IR/Dialect.h"
#include "mlir/IR/Verifier.h"
+#include "mlir/Support/InterfaceSupport.h"
#include "llvm/ADT/DenseMap.h"
#include "llvm/ADT/ScopeExit.h"
#include "llvm/ADT/StringSet.h"
@@ -29,6 +31,7 @@
#include "llvm/Support/PrettyStackTrace.h"
#include "llvm/Support/SourceMgr.h"
#include
+#include
#include
using namespace mlir;
@@ -1443,12 +1446,17 @@ Operation *OperationParser::parseGenericOperation() {
// Try setting the properties for the operation, using a diagnostic to print
// errors.
if (properties) {
- InFlightDiagnostic diagnostic =
- mlir::emitError(srcLocation, "invalid properties ")
- << properties << " for op " << name << ": ";
- if (failed(op->setPropertiesFromAttribute(properties, &diagnostic)))
+ std::unique_ptr diagnostic;
+ auto getDiag = [&]() -> InFlightDiagnostic & {
+ if (!diagnostic) {
+ diagnostic = std::make_unique(
+ mlir::emitError(srcLocation, "invalid properties ")
+ << properties << " for op " << name << ": ");
+ }
+ return *diagnostic;
+ };
+ if (failed(op->setPropertiesFromAttribute(properties, getDiag)))
return nullptr;
- diagnostic.abandon();
}
return op;
@@ -2001,12 +2009,18 @@ OperationParser::parseCustomOperation(ArrayRef resultIDs) {
// Try setting the properties for the operation.
if (properties) {
- InFlightDiagnostic diagnostic =
- mlir::emitError(srcLocation, "invalid properties ")
- << properties << " for op " << op->getName().getStringRef() << ": ";
- if (failed(op->setPropertiesFromAttribute(properties, &diagnostic)))
+ std::unique_ptr diagnostic;
+ auto getDiag = [&]() -> InFlightDiagnostic & {
+ if (!diagnostic) {
+ diagnostic = std::make_unique(
+ mlir::emitError(srcLocation, "invalid properties ")
+ << properties << " for op " << op->getName().getStringRef()
+ << ": ");
+ }
+ return *diagnostic;
+ };
+ if (failed(op->setPropertiesFromAttribute(properties, getDiag)))
return nullptr;
- diagnostic.abandon();
}
return op;
}
diff --git a/mlir/lib/CAPI/IR/IR.cpp b/mlir/lib/CAPI/IR/IR.cpp
index ccdae1424998567..ef234a912490eea 100644
--- a/mlir/lib/CAPI/IR/IR.cpp
+++ b/mlir/lib/CAPI/IR/IR.cpp
@@ -370,16 +370,21 @@ static LogicalResult inferOperationTypes(OperationState &state) {
if (!properties && info->getOpPropertyByteSize() > 0 && !attributes.empty()) {
auto prop = std::make_unique(info->getOpPropertyByteSize());
properties = OpaqueProperties(prop.get());
- InFlightDiagnostic diag = emitError(state.location)
- << " failed properties conversion while building "
- << state.name.getStringRef() << " with `"
- << attributes << "`: ";
- if (failed(info->setOpPropertiesFromAttribute(state.name, properties,
- attributes, &diag))) {
- return failure();
+ if (properties) {
+ std::unique_ptr diagnostic;
+ auto getDiag = [&]() -> InFlightDiagnostic & {
+ if (!diagnostic) {
+ diagnostic = std::make_unique(
+ emitError(state.location)
+ << " failed properties conversion while building "
+ << state.name.getStringRef() << " with `" << attributes << "`: ");
+ }
+ return *diagnostic;
+ };
+ if (failed(info->setOpPropertiesFromAttribute(state.name, properties,
+ attributes, getDiag)))
+ return failure();
}
- diag.abandon();
-
if (succeeded(inferInterface->inferReturnTypes(
context, state.location, state.operands, attributes, properties,
state.regions, state.types))) {
diff --git a/mlir/lib/IR/MLIRContext.cpp b/mlir/lib/IR/MLIRContext.cpp
index e19c3d4d54179b3..5f1d036d22b918e 100644
--- a/mlir/lib/IR/MLIRContext.cpp
+++ b/mlir/lib/IR/MLIRContext.cpp
@@ -852,7 +852,7 @@ void OperationName::UnregisteredOpModel::populateDefaultProperties(
OperationName opName, OpaqueProperties properties) {}
LogicalResult OperationName::UnregisteredOpModel::setPropertiesFromAttr(
OperationName opName, OpaqueProperties properties, Attribute attr,
- InFlightDiagnostic *diag) {
+ function_ref getDiag) {
*properties.as() = attr;
return success();
}
diff --git a/mlir/lib/IR/ODSSupport.cpp b/mlir/lib/IR/ODSSupport.cpp
index f67e7dbf38592e2..0601430c4461651 100644
--- a/mlir/lib/IR/ODSSupport.cpp
+++ b/mlir/lib/IR/ODSSupport.cpp
@@ -18,13 +18,12 @@
using namespace mlir;
-LogicalResult mlir::convertFromAttribute(int64_t &storage,
- ::mlir::Attribute attr,
- ::mlir::InFlightDiagnostic *diag) {
+LogicalResult
+mlir::convertFromAttribute(int64_t &storage, Attribute attr,
+ function_ref getDiag) {
auto valueAttr = dyn_cast(attr);
if (!valueAttr) {
- if (diag)
- *diag << "expected IntegerAttr for key `value`";
+ getDiag() << "expected IntegerAttr for key `value`";
return failure();
}
storage = valueAttr.getValue().getSExtValue();
@@ -35,35 +34,33 @@ Attribute mlir::convertToAttribute(MLIRContext *ctx, int64_t storage) {
}
template
-LogicalResult convertDenseArrayFromAttr(MutableArrayRef storage,
- ::mlir::Attribute attr,
- ::mlir::InFlightDiagnostic *diag,
- StringRef denseArrayTyStr) {
+LogicalResult
+convertDenseArrayFromAttr(MutableArrayRef storage, Attribute attr,
+ function_ref getDiag,
+ StringRef denseArrayTyStr) {
auto valueAttr = dyn_cast(attr);
if (!valueAttr) {
- if (diag)
- *diag << "expected " << denseArrayTyStr << " for key `value`";
+ getDiag() << "expected " << denseArrayTyStr << " for key `value`";
return failure();
}
if (valueAttr.size() != static_cast(storage.size())) {
- if (diag)
- *diag << "size mismatch in attribute conversion: " << valueAttr.size()
- << " vs " << storage.size();
+ getDiag() << "size mismatch in attribute conversion: " << valueAttr.size()
+ << " vs " << storage.size();
return failure();
}
llvm::copy(valueAttr.asArrayRef(), storage.begin());
return success();
}
-LogicalResult mlir::convertFromAttribute(MutableArrayRef storage,
- ::mlir::Attribute attr,
- ::mlir::InFlightDiagnostic *diag) {
- return convertDenseArrayFromAttr(storage, attr, diag,
+LogicalResult
+mlir::convertFromAttribute(MutableArrayRef storage, Attribute attr,
+ function_ref getDiag) {
+ return convertDenseArrayFromAttr(storage, attr, getDiag,
"DenseI64ArrayAttr");
}
-LogicalResult mlir::convertFromAttribute(MutableArrayRef storage,
- Attribute attr,
- InFlightDiagnostic *diag) {
- return convertDenseArrayFromAttr(storage, attr, diag,
+LogicalResult
+mlir::convertFromAttribute(MutableArrayRef storage, Attribute attr,
+ function_ref getDiag) {
+ return convertDenseArrayFromAttr(storage, attr, getDiag,
"DenseI32ArrayAttr");
}
diff --git a/mlir/lib/IR/Operation.cpp b/mlir/lib/IR/Operation.cpp
index ef98a89f4bb49b6..aa577aa089c6860 100644
--- a/mlir/lib/IR/Operation.cpp
+++ b/mlir/lib/IR/Operation.cpp
@@ -351,16 +351,15 @@ Attribute Operation::getPropertiesAsAttribute() {
return *getPropertiesStorage().as();
return info->getOpPropertiesAsAttribute(this);
}
-LogicalResult
-Operation::setPropertiesFromAttribute(Attribute attr,
- InFlightDiagnostic *diagnostic) {
+LogicalResult Operation::setPropertiesFromAttribute(
+ Attribute attr, function_ref getDiag) {
std::optional info = getRegisteredInfo();
if (LLVM_UNLIKELY(!info)) {
*getPropertiesStorage().as() = attr;
return success();
}
return info->setOpPropertiesFromAttribute(
- this->getName(), this->getPropertiesStorage(...
|
AntonRydahl
pushed a commit
to AntonRydahl/llvm-project
that referenced
this pull request
Sep 11, 2023
Instead of eagerly creating a diagnostic that will be discarded in the normal case, switch to lazy initialization on error.
ZijunZhaoCCK
pushed a commit
to ZijunZhaoCCK/llvm-project
that referenced
this pull request
Sep 19, 2023
Instead of eagerly creating a diagnostic that will be discarded in the normal case, switch to lazy initialization on error.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Instead of eagerly creating a diagnostic that will be discarded in the normal case, switch to lazy initialization on error.