[RFC] Enabling Properties for Attribute storage by default

MLIR
1

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?

1 Like
2

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

3

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::rock::detail::TransformingForOpGenericAdaptorBase::Properties.
I’m wondering how to handle DerrivatedAttr it through ‘Properties’?

4

Can you send a patch adding an example with the DerivedAttr to the TestDialect to showcase the issue?

5

Here is the patch: ⚙ D158581 [MLIR] Switch the default for usePropertiesForAttributes (NFC) ; I’d still like to get some reproducer for the DerivedAttr issue :slight_smile:

6 
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.

7

Thanks! Here is the fix: ⚙ D158679 Fix ODS verifier emission for DerivedAttr when Properties are enabled

8

Thank you very much.

1 Like
9

@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!!).

10

@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.