dialects: (builtin) make DenseIntOrFPElementsAttr generic on element type #3492

alexarice · 2024-11-20T16:29:38Z

Started trying to make DenseIntOrFPElementsAttr a TypedAttribute.
TypedAttributes force that the type parameter must appear as a type var generic. I am not sure why this is the case so it would be good to know what the original motivation for that was. To get this to go through I have disabled this check.

I then ended up adding a typevar for the elements to DenseIntOrFPElementsAttr which caused a lot of follow on changes and I am unsure if the result is nicer than what I started with. I think DenseIntOrFPElementsAttr can be made into a TypedAttribute without this type var.

I also imagine that extensive use of isattr is bad?

codecov · 2024-11-20T16:35:15Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.34%. Comparing base (bc5ea1e) to head (93a17b5).

Additional details and impacted files

@@                      Coverage Diff                      @@
##           alexarice/dense-typed-simple    #3492   +/-   ##
=============================================================
  Coverage                         90.34%   90.34%           
=============================================================
  Files                               464      464           
  Lines                             58048    58060   +12     
  Branches                           5549     5549           
=============================================================
+ Hits                              52442    52453   +11     
  Misses                             4179     4179           
- Partials                           1427     1428    +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests

alexarice · 2024-11-21T22:44:48Z

It would be good to get any opinions on this. I'm tempted to see what the non-generic version would look like

superlopuh · 2024-11-22T08:36:58Z

xdsl/dialects/builtin.py

+AnyDenseElement: TypeAlias = IntegerType | IndexType | AnyFloat
+DenseElementT = TypeVar("DenseElementT", bound=AnyDenseElement, covariant=True)
+_DenseElementT = TypeVar("_DenseElementT", bound=AnyDenseElement)


Is this the equivalent of sized in MLIR? Would it make sense to add this independently of the other changes?

I doubt it is as IndexType is not sized right? (Also IndexType should be forbidden from DenseIntOrFPElementsAttr but that's another matter)

I think it's quite relevant, and we should forbid it indeed, I thought we already made that change recently.

Ah it was DenseArrayBase #3258

I think in any case that should be a different PR (which could happen before or after this one)

superlopuh · 2024-11-22T08:39:43Z

xdsl/dialects/builtin.py

+        printer.print_string(">")
+
+
+DenseIntElementsAttr: TypeAlias = DenseIntOrFPElementsAttr[IntegerType]


This line feels like it could have its own PR, we could probably have a custom constraint to start with to minimise the diff

I still feel like this could be its own PR :)

You want a separate PR just for the type alias?

Yeah that was the idea, it technically is its own API surface change and seems to be a large contribution to this PR's diff, independent of the other changes.

I don't see how it makes any impact on the diff at all. Anything that is changed to a DenseIntElementAttr would have had to be changed to a DenseIntOrFPElementsAttr[IntegerType] or at least DenseIntOrFPElementsAttr[AnyDenseElement]

Sorry that's what I mean, this change in the diff of this PR could already be done in main directly, and is not dependent on the changes you make here. Or am I misunderstanding?

I don't see how to separate the changes. I can't change DenseIntOrFPElementsAttr to DenseIntOrFPElementsAttr[AnyDenseElement] before this PR.

I could potentially do two steps of changing every DenseIntOrFPElementsAttr to DenseIntOrFPElementsAttr[AnyDenseElement] and then do a second PR specialising some of these to DenseIntElementsAttr = DenseIntOrFPElementsAttr[IntegerType], but I don't think the first diff will be any smaller.

Ah sorry that's not what I meant, I meant something like

Suggested change

DenseIntElementsAttr: TypeAlias = DenseIntOrFPElementsAttr[IntegerType]

DenseIntElementsAttr: TypeAlias = Annotated[DenseIntOrFPElementsAttr, ParametrizedAttrConstraint(Attribute, (BaseAttr(IntegerType),AnyAttr()))]

Ah I get it now, I could give that a go

superlopuh

I like this overall, and agree that it doesn't necessarily make sense to restrict the type to have the generic parameter. It would be nice to minimise the big diff, if possible, as somme of the changes seem like more obvious improvements than others

alexarice · 2024-11-22T08:57:34Z

I think the first step is to make DenseIntOrFPElementsAttr a TypedAttribute[AnyDenseElement], without giving it a generic parameter. I think the changes should then be localised to the parsing changes and the removal of the generic check on TypedAttributes

superlopuh · 2024-11-22T08:59:57Z

Yep that also crossed my mind, not sure whether there is a single best first step but it would be nice to have a smaller diff for sure.

alexarice · 2024-11-22T09:01:15Z

I'll split that out and see what it looks like

alexarice · 2024-11-22T17:43:32Z

Reworked and stacked this PR. Still not 100% sure this is desirable

…3509) A simpler version of #3492 which does not make `DenseIntOrFPElementsAttr` generic.

superlopuh · 2024-11-27T09:36:54Z

xdsl/interpreters/builtin.py

@@ -85,7 +88,7 @@ def dense_int_or_fp_elements_value(
        attr: Attribute,
        type_attr: builtin.MemRefType[Any],
    ) -> ShapedArray[Any]:
-        assert isinstance(attr, builtin.DenseIntOrFPElementsAttr)
+        assert isattr(attr, base(builtin.DenseIntOrFPElementsAttr[AnyDenseElement]))


I would rather have a AnyDenseIntOrFPElementsAttrConstr TBH

To expand on this thought, it's because base does the type parsing every time this is called, whereas Constr caches that, and is a tiny bit more resilient to API changes in the future

alexarice requested review from superlopuh, math-fehr and PapyChacal November 20, 2024 16:29

alexarice marked this pull request as draft November 20, 2024 16:29

alexarice self-assigned this Nov 20, 2024

alexarice added the dialects Changes on the dialects label Nov 20, 2024

alexarice changed the title ~~[Draft] core: make DenseIntOrFPElementsAttr a TypedAttribute~~ [Draft] dialects: (builtin) make DenseIntOrFPElementsAttr a TypedAttribute Nov 20, 2024

alexarice marked this pull request as ready for review November 21, 2024 22:43

superlopuh reviewed Nov 22, 2024

View reviewed changes

alexarice mentioned this pull request Nov 22, 2024

core: remove generic and generic check from TypedAttribute #3504

Merged

alexarice force-pushed the alexarice/dense-typed branch from 2b2c93b to d468f99 Compare November 22, 2024 09:42

alexarice mentioned this pull request Nov 22, 2024

dialects: (builtin) make DenseIntOrFPElementsAttr a TypedAttribute #3509

Merged

alexarice added 3 commits November 22, 2024 17:31

dialects: (builtin) make DenseIntOrFPElementsAttr a TypedAttribute

a21644a

Remove unused import

bc5ea1e

core: make DenseIntOrFPElementsAttr generic on element type

93a17b5

alexarice force-pushed the alexarice/dense-typed branch from d468f99 to 93a17b5 Compare November 22, 2024 17:41

alexarice changed the base branch from main to alexarice/dense-typed-simple November 22, 2024 17:42

alexarice changed the title ~~[Draft] dialects: (builtin) make DenseIntOrFPElementsAttr a TypedAttribute~~ dialects: (builtin) make DenseIntOrFPElementsAttr generic on element type Nov 22, 2024

alexarice added a commit that referenced this pull request Nov 22, 2024

dialects: (builtin) make DenseIntOrFPElementsAttr a TypedAttribute (#…

018d17f

…3509) A simpler version of #3492 which does not make `DenseIntOrFPElementsAttr` generic.

Base automatically changed from alexarice/dense-typed-simple to main November 22, 2024 23:53

superlopuh reviewed Nov 27, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dialects: (builtin) make DenseIntOrFPElementsAttr generic on element type #3492

dialects: (builtin) make DenseIntOrFPElementsAttr generic on element type #3492

alexarice commented Nov 20, 2024

codecov bot commented Nov 20, 2024 •

edited

Loading

alexarice commented Nov 21, 2024

superlopuh Nov 22, 2024

alexarice Nov 22, 2024

superlopuh Nov 26, 2024

superlopuh Nov 26, 2024

alexarice Nov 26, 2024

superlopuh Nov 22, 2024

superlopuh Nov 22, 2024

alexarice Nov 22, 2024

superlopuh Nov 26, 2024

alexarice Nov 26, 2024

superlopuh Nov 27, 2024

alexarice Nov 27, 2024

superlopuh Nov 27, 2024

alexarice Nov 27, 2024

superlopuh left a comment

alexarice commented Nov 22, 2024

superlopuh commented Nov 22, 2024

alexarice commented Nov 22, 2024

alexarice commented Nov 22, 2024

superlopuh Nov 27, 2024

superlopuh Nov 27, 2024

		printer.print_string(">")


		DenseIntElementsAttr: TypeAlias = DenseIntOrFPElementsAttr[IntegerType]

	DenseIntElementsAttr: TypeAlias = DenseIntOrFPElementsAttr[IntegerType]
	DenseIntElementsAttr: TypeAlias = Annotated[DenseIntOrFPElementsAttr, ParametrizedAttrConstraint(Attribute, (BaseAttr(IntegerType),AnyAttr()))]

dialects: (builtin) make DenseIntOrFPElementsAttr generic on element type #3492

Are you sure you want to change the base?

dialects: (builtin) make DenseIntOrFPElementsAttr generic on element type #3492

Conversation

alexarice commented Nov 20, 2024

codecov bot commented Nov 20, 2024 • edited Loading

Codecov Report

alexarice commented Nov 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

superlopuh left a comment

Choose a reason for hiding this comment

alexarice commented Nov 22, 2024

superlopuh commented Nov 22, 2024

alexarice commented Nov 22, 2024

alexarice commented Nov 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 20, 2024 •

edited

Loading