Skip to content

Commit c50617d

Browse files
joker-ephjoker-eph
authored
Simplify diagnostic error management for MLIR properties API (NFC) (#67409)
This is a follow-up to 8c2bff1 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.
1 parent ac466c7 commit c50617d

File tree

18 files changed

+94
-113
lines changed

18 files changed

+94
-113
lines changed

‎mlir/include/mlir/IR/ExtensibleDialect.h

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -476,7 +476,7 @@ class DynamicOpDefinition : public OperationName::Impl {
476476
void populateInherentAttrs(Operation *op, NamedAttrList &attrs) final {}
477477
LogicalResult
478478
verifyInherentAttrs(OperationName opName, NamedAttrList &attributes,
479-
function_ref<InFlightDiagnostic()> getDiag) final {
479+
function_ref<InFlightDiagnostic()> emitError) final {
480480
return success();
481481
}
482482
int getOpPropertyByteSize() final { return 0; }
@@ -489,8 +489,8 @@ class DynamicOpDefinition : public OperationName::Impl {
489489
LogicalResult
490490
setPropertiesFromAttr(OperationName opName, OpaqueProperties properties,
491491
Attribute attr,
492-
function_ref<InFlightDiagnostic &()> getDiag) final {
493-
getDiag() << "extensible Dialects don't support properties";
492+
function_ref<InFlightDiagnostic()> emitError) final {
493+
emitError() << "extensible Dialects don't support properties";
494494
return failure();
495495
}
496496
Attribute getPropertiesAsAttr(Operation *op) final { return {}; }

‎mlir/include/mlir/IR/ODSSupport.h

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ namespace mlir {
2828
/// error message is also emitted.
2929
LogicalResult
3030
convertFromAttribute(int64_t &storage, Attribute attr,
31-
function_ref<InFlightDiagnostic &()> getDiag);
31+
function_ref<InFlightDiagnostic()> emitError);
3232

3333
/// Convert the provided int64_t to an IntegerAttr attribute.
3434
Attribute convertToAttribute(MLIRContext *ctx, int64_t storage);
@@ -39,15 +39,15 @@ Attribute convertToAttribute(MLIRContext *ctx, int64_t storage);
3939
/// the optional diagnostic is provided an error message is also emitted.
4040
LogicalResult
4141
convertFromAttribute(MutableArrayRef<int64_t> storage, Attribute attr,
42-
function_ref<InFlightDiagnostic &()> getDiag);
42+
function_ref<InFlightDiagnostic()> emitError);
4343

4444
/// Convert a DenseI32ArrayAttr to the provided storage. It is expected that the
4545
/// storage has the same size as the array. An error is returned if the
4646
/// attribute isn't a DenseI32ArrayAttr or it does not have the same size. If
4747
/// the optional diagnostic is provided an error message is also emitted.
4848
LogicalResult
4949
convertFromAttribute(MutableArrayRef<int32_t> storage, Attribute attr,
50-
function_ref<InFlightDiagnostic &()> getDiag);
50+
function_ref<InFlightDiagnostic()> emitError);
5151

5252
/// Convert the provided ArrayRef<int64_t> to a DenseI64ArrayAttr attribute.
5353
Attribute convertToAttribute(MLIRContext *ctx, ArrayRef<int64_t> storage);

‎mlir/include/mlir/IR/OpDefinition.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1741,8 +1741,8 @@ class Op : public OpState, public Traits<ConcreteType>... {
17411741
template <typename PropertiesTy>
17421742
static LogicalResult
17431743
setPropertiesFromAttr(PropertiesTy &prop, Attribute attr,
1744-
function_ref<InFlightDiagnostic &()> getDiag) {
1745-
return setPropertiesFromAttribute(prop, attr, getDiag);
1744+
function_ref<InFlightDiagnostic()> emitError) {
1745+
return setPropertiesFromAttribute(prop, attr, emitError);
17461746
}
17471747
/// Convert the provided properties to an attribute. This default
17481748
/// implementation forwards to a free function `getPropertiesAsAttribute` that

‎mlir/include/mlir/IR/Operation.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -901,7 +901,7 @@ class alignas(8) Operation final
901901
/// generic format. An optional diagnostic can be passed in for richer errors.
902902
LogicalResult
903903
setPropertiesFromAttribute(Attribute attr,
904-
function_ref<InFlightDiagnostic &()> getDiag);
904+
function_ref<InFlightDiagnostic()> emitError);
905905

906906
/// Copy properties from an existing other properties object. The two objects
907907
/// must be the same type.

‎mlir/include/mlir/IR/OperationSupport.h

Lines changed: 15 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -129,7 +129,7 @@ class OperationName {
129129
virtual void populateInherentAttrs(Operation *op, NamedAttrList &attrs) = 0;
130130
virtual LogicalResult
131131
verifyInherentAttrs(OperationName opName, NamedAttrList &attributes,
132-
function_ref<InFlightDiagnostic()> getDiag) = 0;
132+
function_ref<InFlightDiagnostic()> emitError) = 0;
133133
virtual int getOpPropertyByteSize() = 0;
134134
virtual void initProperties(OperationName opName, OpaqueProperties storage,
135135
OpaqueProperties init) = 0;
@@ -138,7 +138,7 @@ class OperationName {
138138
OpaqueProperties properties) = 0;
139139
virtual LogicalResult
140140
setPropertiesFromAttr(OperationName, OpaqueProperties, Attribute,
141-
function_ref<InFlightDiagnostic &()> getDiag) = 0;
141+
function_ref<InFlightDiagnostic()> emitError) = 0;
142142
virtual Attribute getPropertiesAsAttr(Operation *) = 0;
143143
virtual void copyProperties(OpaqueProperties, OpaqueProperties) = 0;
144144
virtual bool compareProperties(OpaqueProperties, OpaqueProperties) = 0;
@@ -210,7 +210,7 @@ class OperationName {
210210
void populateInherentAttrs(Operation *op, NamedAttrList &attrs) final;
211211
LogicalResult
212212
verifyInherentAttrs(OperationName opName, NamedAttrList &attributes,
213-
function_ref<InFlightDiagnostic()> getDiag) final;
213+
function_ref<InFlightDiagnostic()> emitError) final;
214214
int getOpPropertyByteSize() final;
215215
void initProperties(OperationName opName, OpaqueProperties storage,
216216
OpaqueProperties init) final;
@@ -219,7 +219,7 @@ class OperationName {
219219
OpaqueProperties properties) final;
220220
LogicalResult
221221
setPropertiesFromAttr(OperationName, OpaqueProperties, Attribute,
222-
function_ref<InFlightDiagnostic &()> getDiag) final;
222+
function_ref<InFlightDiagnostic()> emitError) final;
223223
Attribute getPropertiesAsAttr(Operation *) final;
224224
void copyProperties(OpaqueProperties, OpaqueProperties) final;
225225
bool compareProperties(OpaqueProperties, OpaqueProperties) final;
@@ -408,8 +408,8 @@ class OperationName {
408408
/// attributes when parsed from the older generic syntax pre-Properties.
409409
LogicalResult
410410
verifyInherentAttrs(NamedAttrList &attributes,
411-
function_ref<InFlightDiagnostic()> getDiag) const {
412-
return getImpl()->verifyInherentAttrs(*this, attributes, getDiag);
411+
function_ref<InFlightDiagnostic()> emitError) const {
412+
return getImpl()->verifyInherentAttrs(*this, attributes, emitError);
413413
}
414414
/// This hooks return the number of bytes to allocate for the op properties.
415415
int getOpPropertyByteSize() const {
@@ -439,8 +439,9 @@ class OperationName {
439439
/// Define the op properties from the provided Attribute.
440440
LogicalResult setOpPropertiesFromAttribute(
441441
OperationName opName, OpaqueProperties properties, Attribute attr,
442-
function_ref<InFlightDiagnostic &()> getDiag) const {
443-
return getImpl()->setPropertiesFromAttr(opName, properties, attr, getDiag);
442+
function_ref<InFlightDiagnostic()> emitError) const {
443+
return getImpl()->setPropertiesFromAttr(opName, properties, attr,
444+
emitError);
444445
}
445446

446447
void copyOpProperties(OpaqueProperties lhs, OpaqueProperties rhs) const {
@@ -596,9 +597,9 @@ class RegisteredOperationName : public OperationName {
596597
}
597598
LogicalResult
598599
verifyInherentAttrs(OperationName opName, NamedAttrList &attributes,
599-
function_ref<InFlightDiagnostic()> getDiag) final {
600+
function_ref<InFlightDiagnostic()> emitError) final {
600601
if constexpr (hasProperties)
601-
return ConcreteOp::verifyInherentAttrs(opName, attributes, getDiag);
602+
return ConcreteOp::verifyInherentAttrs(opName, attributes, emitError);
602603
return success();
603604
}
604605
// Detect if the concrete operation defined properties.
@@ -636,12 +637,12 @@ class RegisteredOperationName : public OperationName {
636637
LogicalResult
637638
setPropertiesFromAttr(OperationName opName, OpaqueProperties properties,
638639
Attribute attr,
639-
function_ref<InFlightDiagnostic &()> getDiag) final {
640+
function_ref<InFlightDiagnostic()> emitError) final {
640641
if constexpr (hasProperties) {
641642
auto p = properties.as<Properties *>();
642-
return ConcreteOp::setPropertiesFromAttr(*p, attr, getDiag);
643+
return ConcreteOp::setPropertiesFromAttr(*p, attr, emitError);
643644
}
644-
getDiag() << "this operation does not support properties";
645+
emitError() << "this operation does not support properties";
645646
return failure();
646647
}
647648
Attribute getPropertiesAsAttr(Operation *op) final {
@@ -1010,7 +1011,7 @@ struct OperationState {
10101011
// optionally emit diagnostics on error through the provided diagnostic.
10111012
LogicalResult
10121013
setProperties(Operation *op,
1013-
function_ref<InFlightDiagnostic &()> getDiag) const;
1014+
function_ref<InFlightDiagnostic()> emitError) const;
10141015

10151016
void addOperands(ValueRange newOperands);
10161017

‎mlir/lib/AsmParser/Parser.cpp

Lines changed: 9 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1446,16 +1446,11 @@ Operation *OperationParser::parseGenericOperation() {
14461446
// Try setting the properties for the operation, using a diagnostic to print
14471447
// errors.
14481448
if (properties) {
1449-
std::unique_ptr<InFlightDiagnostic> diagnostic;
1450-
auto getDiag = [&]() -> InFlightDiagnostic & {
1451-
if (!diagnostic) {
1452-
diagnostic = std::make_unique<InFlightDiagnostic>(
1453-
mlir::emitError(srcLocation, "invalid properties ")
1454-
<< properties << " for op " << name << ": ");
1455-
}
1456-
return *diagnostic;
1449+
auto emitError = [&]() {
1450+
return mlir::emitError(srcLocation, "invalid properties ")
1451+
<< properties << " for op " << name << ": ";
14571452
};
1458-
if (failed(op->setPropertiesFromAttribute(properties, getDiag)))
1453+
if (failed(op->setPropertiesFromAttribute(properties, emitError)))
14591454
return nullptr;
14601455
}
14611456

@@ -2009,17 +2004,12 @@ OperationParser::parseCustomOperation(ArrayRef<ResultRecord> resultIDs) {
20092004

20102005
// Try setting the properties for the operation.
20112006
if (properties) {
2012-
std::unique_ptr<InFlightDiagnostic> diagnostic;
2013-
auto getDiag = [&]() -> InFlightDiagnostic & {
2014-
if (!diagnostic) {
2015-
diagnostic = std::make_unique<InFlightDiagnostic>(
2016-
mlir::emitError(srcLocation, "invalid properties ")
2017-
<< properties << " for op " << op->getName().getStringRef()
2018-
<< ": ");
2019-
}
2020-
return *diagnostic;
2007+
auto emitError = [&]() {
2008+
return mlir::emitError(srcLocation, "invalid properties ")
2009+
<< properties << " for op " << op->getName().getStringRef()
2010+
<< ": ";
20212011
};
2022-
if (failed(op->setPropertiesFromAttribute(properties, getDiag)))
2012+
if (failed(op->setPropertiesFromAttribute(properties, emitError)))
20232013
return nullptr;
20242014
}
20252015
return op;

‎mlir/lib/CAPI/IR/IR.cpp

Lines changed: 5 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -416,18 +416,13 @@ static LogicalResult inferOperationTypes(OperationState &state) {
416416
auto prop = std::make_unique<char[]>(info->getOpPropertyByteSize());
417417
properties = OpaqueProperties(prop.get());
418418
if (properties) {
419-
std::unique_ptr<InFlightDiagnostic> diagnostic;
420-
auto getDiag = [&]() -> InFlightDiagnostic & {
421-
if (!diagnostic) {
422-
diagnostic = std::make_unique<InFlightDiagnostic>(
423-
emitError(state.location)
424-
<< " failed properties conversion while building "
425-
<< state.name.getStringRef() << " with `" << attributes << "`: ");
426-
}
427-
return *diagnostic;
419+
auto emitError = [&]() {
420+
return mlir::emitError(state.location)
421+
<< " failed properties conversion while building "
422+
<< state.name.getStringRef() << " with `" << attributes << "`: ";
428423
};
429424
if (failed(info->setOpPropertiesFromAttribute(state.name, properties,
430-
attributes, getDiag)))
425+
attributes, emitError)))
431426
return failure();
432427
}
433428
if (succeeded(inferInterface->inferReturnTypes(

‎mlir/lib/IR/MLIRContext.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -834,7 +834,7 @@ void OperationName::UnregisteredOpModel::populateInherentAttrs(
834834
Operation *op, NamedAttrList &attrs) {}
835835
LogicalResult OperationName::UnregisteredOpModel::verifyInherentAttrs(
836836
OperationName opName, NamedAttrList &attributes,
837-
function_ref<InFlightDiagnostic()> getDiag) {
837+
function_ref<InFlightDiagnostic()> emitError) {
838838
return success();
839839
}
840840
int OperationName::UnregisteredOpModel::getOpPropertyByteSize() {
@@ -852,7 +852,7 @@ void OperationName::UnregisteredOpModel::populateDefaultProperties(
852852
OperationName opName, OpaqueProperties properties) {}
853853
LogicalResult OperationName::UnregisteredOpModel::setPropertiesFromAttr(
854854
OperationName opName, OpaqueProperties properties, Attribute attr,
855-
function_ref<InFlightDiagnostic &()> getDiag) {
855+
function_ref<InFlightDiagnostic()> emitError) {
856856
*properties.as<Attribute *>() = attr;
857857
return success();
858858
}

‎mlir/lib/IR/ODSSupport.cpp

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -20,10 +20,10 @@ using namespace mlir;
2020

2121
LogicalResult
2222
mlir::convertFromAttribute(int64_t &storage, Attribute attr,
23-
function_ref<InFlightDiagnostic &()> getDiag) {
23+
function_ref<InFlightDiagnostic()> emitError) {
2424
auto valueAttr = dyn_cast<IntegerAttr>(attr);
2525
if (!valueAttr) {
26-
getDiag() << "expected IntegerAttr for key `value`";
26+
emitError() << "expected IntegerAttr for key `value`";
2727
return failure();
2828
}
2929
storage = valueAttr.getValue().getSExtValue();
@@ -36,31 +36,31 @@ Attribute mlir::convertToAttribute(MLIRContext *ctx, int64_t storage) {
3636
template <typename DenseArrayTy, typename T>
3737
LogicalResult
3838
convertDenseArrayFromAttr(MutableArrayRef<T> storage, Attribute attr,
39-
function_ref<InFlightDiagnostic &()> getDiag,
39+
function_ref<InFlightDiagnostic()> emitError,
4040
StringRef denseArrayTyStr) {
4141
auto valueAttr = dyn_cast<DenseArrayTy>(attr);
4242
if (!valueAttr) {
43-
getDiag() << "expected " << denseArrayTyStr << " for key `value`";
43+
emitError() << "expected " << denseArrayTyStr << " for key `value`";
4444
return failure();
4545
}
4646
if (valueAttr.size() != static_cast<int64_t>(storage.size())) {
47-
getDiag() << "size mismatch in attribute conversion: " << valueAttr.size()
48-
<< " vs " << storage.size();
47+
emitError() << "size mismatch in attribute conversion: " << valueAttr.size()
48+
<< " vs " << storage.size();
4949
return failure();
5050
}
5151
llvm::copy(valueAttr.asArrayRef(), storage.begin());
5252
return success();
5353
}
5454
LogicalResult
5555
mlir::convertFromAttribute(MutableArrayRef<int64_t> storage, Attribute attr,
56-
function_ref<InFlightDiagnostic &()> getDiag) {
57-
return convertDenseArrayFromAttr<DenseI64ArrayAttr>(storage, attr, getDiag,
56+
function_ref<InFlightDiagnostic()> emitError) {
57+
return convertDenseArrayFromAttr<DenseI64ArrayAttr>(storage, attr, emitError,
5858
"DenseI64ArrayAttr");
5959
}
6060
LogicalResult
6161
mlir::convertFromAttribute(MutableArrayRef<int32_t> storage, Attribute attr,
62-
function_ref<InFlightDiagnostic &()> getDiag) {
63-
return convertDenseArrayFromAttr<DenseI32ArrayAttr>(storage, attr, getDiag,
62+
function_ref<InFlightDiagnostic()> emitError) {
63+
return convertDenseArrayFromAttr<DenseI32ArrayAttr>(storage, attr, emitError,
6464
"DenseI32ArrayAttr");
6565
}
6666

‎mlir/lib/IR/Operation.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -352,14 +352,14 @@ Attribute Operation::getPropertiesAsAttribute() {
352352
return info->getOpPropertiesAsAttribute(this);
353353
}
354354
LogicalResult Operation::setPropertiesFromAttribute(
355-
Attribute attr, function_ref<InFlightDiagnostic &()> getDiag) {
355+
Attribute attr, function_ref<InFlightDiagnostic()> emitError) {
356356
std::optional<RegisteredOperationName> info = getRegisteredInfo();
357357
if (LLVM_UNLIKELY(!info)) {
358358
*getPropertiesStorage().as<Attribute *>() = attr;
359359
return success();
360360
}
361361
return info->setOpPropertiesFromAttribute(
362-
this->getName(), this->getPropertiesStorage(), attr, getDiag);
362+
this->getName(), this->getPropertiesStorage(), attr, emitError);
363363
}
364364

365365
void Operation::copyProperties(OpaqueProperties rhs) {

‎mlir/lib/IR/OperationSupport.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -199,10 +199,10 @@ OperationState::~OperationState() {
199199
}
200200

201201
LogicalResult OperationState::setProperties(
202-
Operation *op, function_ref<InFlightDiagnostic &()> getDiag) const {
202+
Operation *op, function_ref<InFlightDiagnostic()> emitError) const {
203203
if (LLVM_UNLIKELY(propertiesAttr)) {
204204
assert(!properties);
205-
return op->setPropertiesFromAttribute(propertiesAttr, getDiag);
205+
return op->setPropertiesFromAttribute(propertiesAttr, emitError);
206206
}
207207
if (properties)
208208
propertiesSetter(op->getPropertiesStorage(), properties);

‎mlir/lib/TableGen/CodeGenHelpers.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -133,9 +133,9 @@ static ::mlir::LogicalResult {0}(
133133
/// functions are stripped anyways.
134134
static const char *const attrConstraintCode = R"(
135135
static ::mlir::LogicalResult {0}(
136-
::mlir::Attribute attr, ::llvm::StringRef attrName, llvm::function_ref<::mlir::InFlightDiagnostic()> getDiag) {{
136+
::mlir::Attribute attr, ::llvm::StringRef attrName, llvm::function_ref<::mlir::InFlightDiagnostic()> emitError) {{
137137
if (attr && !({1}))
138-
return getDiag() << "attribute '" << attrName
138+
return emitError() << "attribute '" << attrName
139139
<< "' failed to satisfy constraint: {2}";
140140
return ::mlir::success();
141141
}

0 commit comments

Comments
 (0)