I’d like to start switching the default let usePropertiesForAttributes = 1; for ODS Dialects definition, the opt-out will still be possible of course.
This has been deployed in quite a few places without fallouts that I know of. I’m wondering if there is any blocker or missing feature right now related to this?
I have a sense (I’ll need to check with the relevant coworker) that we’re having trouble swapping to attributes-as-properties because of DerivedAttr
I have the problem when add in our dialect definition, let usePropertiesForAttributes = 1.
My build was failed, in operation definition i have DerivedAttr upperLen, and get error: no member named upperLen in mlir:
:detail::TransformingForOpGenericAdaptorBase::Properties.
I’m wondering how to handle DerrivatedAttr it through ‘Properties’?
Can you send a patch adding an example with the DerivedAttr to the TestDialect to showcase the issue?
Here is the patch: ⚙ D158581 [MLIR] Switch the default for usePropertiesForAttributes (NFC) ; I’d still like to get some reproducer for the DerivedAttr issue 
diff --git a/mlir/test/lib/Dialect/Test/TestOps.td b/mlir/test/lib/Dialect/Test/TestOps.td
index 373aeebd0a76..0ba82f706c3e 100644
--- a/mlir/test/lib/Dialect/Test/TestOps.td
+++ b/mlir/test/lib/Dialect/Test/TestOps.td
@@ -1815,6 +1815,18 @@ def TestReturnOp : TEST_Op<"return", [ReturnLike, Terminator]> {
[{ build($_builder, $_state, {}); }]>
];
}
+
+
+def TestPropOp : TEST_Op<"prop">,
+ Arguments<(ins Variadic<Index>:$upperInits,
+ I32ElementsAttr:$transforms)>,
+ Results<(outs Variadic<AnyType>:$results)>
+ {
+ DerivedAttr upperLen = DerivedAttr<"uint32_t", [{
+ return getUpperInits().size() / getTransforms().size();
+ }], [{ $_builder.getI32IntegerAttr($_self) }]>;
+ }
+
def TestCastOp : TEST_Op<"cast">,
Arguments<(ins Variadic<AnyType>)>, Results<(outs AnyType)>;
def TestInvalidOp : TEST_Op<"invalid", [Terminator]>,
(END)
Do you wish sent this via phabricator?
I thought this will be faster.
Thanks! Here is the fix: ⚙ D158679 Fix ODS verifier emission for DerivedAttr when Properties are enabled
Thank you very much.
@krzysz00 let me know if that it is still a blocker on your side or if this is enough to fix your use cases!
Also I want to stress that this change is only changing the default, if any issue is found downstream, dialects can still opt-out into staying with the current behavior for now (but please file bugs!!).
@mehdi_amini We’ll hopefully have word in a day or two on whether that change fixes it (I’m buried in other work and so can’t test the backport personally)
In any case, I think you can go ahead with the enablement - I raised this now so that it’d get fixed before the inevitable “drop support for the old way of handling inherent attributes” PR several months down the line.
Hi!
I think I found a bug, (after setting: let usePropertiesForAttributes = 0 my code works).
I have my own dialect, and one of the ops has the InferTypeOpAdaptor interface.
The op also has some attributes that are saved as properties.
Then another builder is generated, without the result type:
void MyOp::build(::mlir::OpBuilder &odsBuilder, ::mlir::OperationState &odsState, ::mlir::ValueRange operands, ::llvm::ArrayRef<::mlir::NamedAttribute> attributes) {
assert(operands.size() == 3u && "mismatched number of parameters");
odsState.addOperands(operands);
odsState.addAttributes(attributes);
::llvm::SmallVector<::mlir::Type, 2> inferredReturnTypes;
if (::mlir::succeeded(MyOp::inferReturnTypes(odsBuilder.getContext(),
odsState.location, operands,
odsState.attributes.getDictionary(odsState.getContext()),
odsState.getRawProperties(),
odsState.regions, inferredReturnTypes))) {
assert(inferredReturnTypes.size() == 1u && "mismatched number of return types");
odsState.addTypes(inferredReturnTypes);
} else {
::llvm::report_fatal_error("Failed to infer result type(s).");
}
}
::mlir::LogicalResult
MyOp::inferReturnTypes(::mlir::MLIRContext *context,
std::optional<::mlir::Location> location,
::mlir::ValueRange operands, ::mlir::DictionaryAttr attributes,
::mlir::OpaqueProperties properties, ::mlir::RegionRange regions,
::llvm::SmallVectorImpl<::mlir::Type> &inferredReturnTypes) {
MyOp::Adaptor adaptor(operands, attributes, properties, regions);
return MyOp::inferReturnTypes(context,
location, adaptor, inferredReturnTypes);
}
Where the last MyOp::inferReturnTypes is implemented by me.
When I use this builder the code crashes. The adaptor doesn’t have the properties set.
So my MyOp::inferReturnTypes method crashes when trying to read a property. I cannot read it from the attributes because the get....AttrName is the op’s method.
I think the generated code had to somehow set the properties using the attributes.
Something similar to the generated code MyOp::setPropertiesFromAttr, but this is the op’s method and not the adaptor’s.
Thanks in advance,
Maya
That’s weird, the generated code seems to be doing the right thing (it’s passing properties to the adaptor). Could you check what the properties is being passed to the build method?
This build method may look indeed unsafe if this op expects inherent attributes: it should take them explicitly otherwise the properties won’t be set before calling the type inference.
@jpienaar @mehdi_amini
My op expects inherent attributes, which as you mentioned are not set.
I call the build method and pass these attributes as the attributes.
I expect the generated method to take the relevant attributes from attributes and set them as properties.
I don’t mind fixing it after we agree what the generated code should be.
It likely should call setPropertiesFromAttribute using the operation name but it’s not straightforward either. This may be instead the kind of builder to remove when using properties.