dialects: (csl) builtin math lib for DSDs #2686

n-io · 2024-06-03T15:58:44Z

DSD builtin operations typically come with a variety of overloads. This PR proposes a base class handling ops and verification. The subclasses correspond to CSL builtin ops. Their implementation only needs to specify op name and the valid function signatures. The goal is to keep everything really simple.

…per case

codecov · 2024-06-04T09:09:58Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.74%. Comparing base (a83b98b) to head (34983d3).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2686      +/-   ##
==========================================
+ Coverage   89.70%   89.74%   +0.04%     
==========================================
  Files         366      367       +1     
  Lines       47071    47285     +214     
  Branches     7140     7191      +51     
==========================================
+ Hits        42223    42437     +214     
  Misses       3745     3745              
  Partials     1103     1103

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

dk949

I am still not sure about implementing a class for every version of each
builtin. I agree that this is the simpler solution to initially implement, I
feel like it will be harder to maintain and a bit more difficult to transform.

I think it should be possible to roll any number of builtins into the same
class. The two ideas I had yesterday were to:

Provide the suffixes as part of the signature and a public function for the
printer to call, performing a match and returning the associated suffix. This
method would be quite easy to implement, but it adds printer specific
functionality to the dialect, which is undesirable.
Derive the suffix from the signature(s). There appears to be some sort of a
system for naming these builtins, however, some functions are
indistinguishable by signature alone, as they use untyped DSDs.
(This is the 3rd of the two ideas). Disregard concerns in 1st paragraph and
implement each builtin as its own class 🤷

xdsl/dialects/csl.py

dk949 · 2024-06-04T10:08:10Z

xdsl/dialects/csl.py

+    def verify_(self) -> None:
+        def typcheck(
+            op_typ: Attribute,
+            sig_typ: AttrConstraint | Attribute | type[Attribute] | TypeVar,


I am not sure we need AttrConstraint here, since we don't really use them anywhere else in the dialect (not directly any way). If you do think we should keep it, maybe consider using attr_constr_coercion from irdl, to convert everything to an AttrConstraint then using the verify method.

It's the type signature copied over from operand_def

dk949 · 2024-06-04T10:08:26Z

xdsl/dialects/csl.py

@@ -883,6 +892,50 @@ def verify_(self) -> None:
            raise VerifyException(f"{self.name} can only operate on mem1d_dsd type")


+FunctionSignatures = list[
+    tuple[AttrConstraint | Attribute | type[Attribute] | TypeVar, ...]


Ditto for use of AttrConstraint.

n-io · 2024-06-04T15:25:11Z

I am still not sure about implementing a class for every version of each builtin. I agree that this is the simpler solution to initially implement, I feel like it will be harder to maintain and a bit more difficult to transform.

I agree, but regrettably haven't really gotten to that part yet. Happy to include it in this PR if you like.

I think it should be possible to roll any number of builtins into the same class. The two ideas I had yesterday were to:

Provide the suffixes as part of the signature and a public function for the
printer to call, performing a match and returning the associated suffix. This
method would be quite easy to implement, but it adds printer specific
functionality to the dialect, which is undesirable.

This may not really work with the current design, since the signature is based off of the var args, and we can't pass the precision as an operand. However, what we could do is pass the precision as an operand. Then in the get_signatures method we could check the precision property value and return the correct set of signatures. Do you think we want to provide a method for the name as well or do we figure it out in the printer?

Derive the suffix from the signature(s). There appears to be some sort of a
system for naming these builtins, however, some functions are
indistinguishable by signature alone, as they use untyped DSDs.

This would be really really good - however, I do see practical problems. For instance, if we get (DsdType, DsdType, DsdType) as the function signature, this would match both faddh, faddhs, and fadds, since we don't carry around element type information on the DSDs (anymore).

(This is the 3rd of the two ideas). Disregard concerns in 1st paragraph and
implement each builtin as its own class 🤷

Yes, I suppose it's between this and option 1.

AntonLydike · 2024-06-05T12:32:37Z

So I guess to summarise previous discussion:

having one generic csl.dsd_builtin(%a, %b, %c) <{name = "faddh", precision = ...}> : (...) -> ... would be painful and complex to verify?
having one class per "name" and precision might be excessive
having one class per op "name" with precision as a property seems to be the sweet spot?

I would favour having one op per function name emitted in the resulting CSL, except if there's a good reason not to.

AntonLydike

🥹 Can we split this into two or three PRs? This is quite a lot to review all at once

AntonLydike · 2024-06-05T13:51:33Z

xdsl/dialects/csl.py

+            case BuiltinOpPrecisionKind.mixed:
+                return []


This is a way of saying "I support no mixed-precision modes", right?

It will cause it to fail the verification performed by the superclass

xdsl/dialects/csl.py

dk949 · 2024-06-05T13:58:27Z

@AntonLydike

I would favour having one op per function name emitted in the resulting CSL, except if there's a good reason not to.

This is one reason...

I wasn't proposing one single op to rule them all. But maybe all additions together, all macs together etc..

dk949 · 2024-06-05T14:00:40Z

@n-io i really appreciate you adding all the possible DSD operations, but we will realistically never use 90% of these...

I don't think I've ever used a clz in regular code, let alone CSL 😅

n-io · 2024-06-05T14:13:12Z

@n-io i really appreciate you adding all the possible DSD operations, but we will realistically never use 90% of these...

I don't think I've ever used a clz in regular code, let alone CSL 😅

Easy to do, but dito for control tasks 😂

n-io · 2024-06-05T14:26:45Z

If we want one op per function name emitted, that's done quickly and I'm happy to do it. It may actually not be a bad approach going forward, I suppose. We had a similar discussion regarding tasks, where in the end we decided to combine all types of tasks into one parameterised class. I'm happy either way, to be honest.

One op to rule them all might actually make op rewriting rather complicated, also making it generally a lot harder to easily access common functionalities that xdsl provides us with.

AntonLydike

Yeah, lgtm

AntonLydike · 2024-06-06T13:16:30Z

xdsl/dialects/csl.py

+    def verify_(self) -> None:
+        def typcheck(
+            op_typ: Attribute,
+            sig_typ: AttrConstraint | Attribute | type[Attribute] | TypeVar,
+        ) -> bool:
+            if isinstance(sig_typ, type):
+                return isinstance(op_typ, sig_typ)
+            else:
+                return op_typ == sig_typ
+
+        for sig in self.SIGNATURES:
+            if len(self.ops) == len(sig):
+                if all(typcheck(op.type, sig_t) for (op, sig_t) in zip(self.ops, sig)):
+                    return
+        raise VerifyException("Cannot find matching type signature")


Can you add a unit test for this (just in python, not filecheck). I think this would increase confidence here

Added a python test as suggested.

dk949

It's 👍 from me as well, except the AttrConstraint thing in the type checking function

This reverts commit 672a5c7

n-io · 2024-06-06T14:40:57Z

It's 👍 from me as well, except the AttrConstraint thing in the type checking function

It was the full union definition used by operand_def which I thought might be what we wanted. But since we're not actually exposing anything here, you're completely right that we don't technically need it. Removed as suggested.

n-io added 3 commits June 3, 2024 17:50

adding function signature verification mechanism for dsd builtins

54d1440

allowing ClassVar to not throw a wrong_field_exception

afa8b94

change req for suppressing wrong_field_exception to field_name all up…

865ac2e

…per case

n-io requested a review from math-fehr June 4, 2024 09:08

n-io changed the title ~~Dialects: (csl) Adding builtin ops for DSDs~~ core: allow uppercase fields on ops to enable... dialects: (csl) builtin math lib for DSDs Jun 4, 2024

n-io requested a review from AntonLydike June 4, 2024 09:14

n-io marked this pull request as ready for review June 4, 2024 09:14

n-io and others added 4 commits June 4, 2024 11:16

removing comments

92e4393

Merge branch 'main' into nicolai/dsd-math-ops

9868471

using method instead

9531ac9

undoing changes to irdl.py

8956cac

n-io changed the title ~~core: allow uppercase fields on ops to enable... dialects: (csl) builtin math lib for DSDs~~ dialects: (csl) builtin math lib for DSDs Jun 4, 2024

n-io requested review from dk949 and removed request for math-fehr June 4, 2024 09:32

n-io marked this pull request as draft June 4, 2024 10:10

dk949 reviewed Jun 4, 2024

View reviewed changes

n-io added 5 commits June 4, 2024 17:34

Merge branch 'main' into nicolai/dsd-math-ops

cde8837

adding multi-precision ops

df894cc

fixing test

ee8c982

Adding dsd op builtin functions

f238db5

adding Unary16BitOp base class

fcfec0b

n-io added 2 commits June 5, 2024 14:38

cleanup and renaming

bdc117c

adding tests

c0dbdf0

n-io marked this pull request as ready for review June 5, 2024 13:39

AntonLydike reviewed Jun 5, 2024

View reviewed changes

xdsl/dialects/csl.py Outdated Show resolved Hide resolved

n-io added 2 commits June 5, 2024 16:07

removing comments

f690f26

beautifying source code

674f97e

n-io marked this pull request as draft June 5, 2024 16:09

n-io and others added 2 commits June 6, 2024 13:51

Merge branch 'main' into nicolai/dsd-math-ops

905dc92

implementing one op per emitted func

05d328d

n-io marked this pull request as ready for review June 6, 2024 11:54

n-io requested review from AntonLydike and dk949 June 6, 2024 11:55

math-fehr added the dialects Changes on the dialects label Jun 6, 2024

making builtin op base classes non-private

fa0e08c

AntonLydike approved these changes Jun 6, 2024

View reviewed changes

AntonLydike self-assigned this Jun 6, 2024

AntonLydike reviewed Jun 6, 2024

View reviewed changes

dk949 reviewed Jun 6, 2024

View reviewed changes

n-io added 4 commits June 6, 2024 16:22

adding python tests for builtin op verification

672a5c7

actually adding python tests for builtin op verification

312c4c3

Revert incorrect "adding python tests for builtin op verification"

1cd3979

This reverts commit 672a5c7

removing AttrConstraint and TypeVar

34983d3

n-io merged commit 6095319 into main Jun 6, 2024
10 checks passed

n-io deleted the nicolai/dsd-math-ops branch June 6, 2024 14:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dialects: (csl) builtin math lib for DSDs #2686

dialects: (csl) builtin math lib for DSDs #2686

n-io commented Jun 3, 2024 •

edited

Loading

codecov bot commented Jun 4, 2024 •

edited

Loading

dk949 left a comment

dk949 Jun 4, 2024

n-io Jun 4, 2024

dk949 Jun 4, 2024

n-io commented Jun 4, 2024 •

edited

Loading

AntonLydike commented Jun 5, 2024 •

edited

Loading

AntonLydike left a comment

AntonLydike Jun 5, 2024

n-io Jun 5, 2024

n-io Jun 5, 2024

dk949 commented Jun 5, 2024

dk949 commented Jun 5, 2024

n-io commented Jun 5, 2024

n-io commented Jun 5, 2024

AntonLydike left a comment

AntonLydike Jun 6, 2024

n-io Jun 6, 2024

dk949 left a comment

n-io commented Jun 6, 2024

dialects: (csl) builtin math lib for DSDs #2686

dialects: (csl) builtin math lib for DSDs #2686

Conversation

n-io commented Jun 3, 2024 • edited Loading

codecov bot commented Jun 4, 2024 • edited Loading

Codecov Report

dk949 left a 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

n-io commented Jun 4, 2024 • edited Loading

AntonLydike commented Jun 5, 2024 • edited Loading

AntonLydike left a 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

dk949 commented Jun 5, 2024

dk949 commented Jun 5, 2024

n-io commented Jun 5, 2024

n-io commented Jun 5, 2024

AntonLydike left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dk949 left a comment

Choose a reason for hiding this comment

n-io commented Jun 6, 2024

n-io commented Jun 3, 2024 •

edited

Loading

codecov bot commented Jun 4, 2024 •

edited

Loading

n-io commented Jun 4, 2024 •

edited

Loading

AntonLydike commented Jun 5, 2024 •

edited

Loading