Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dialects: (linalg) add correct generic printing for fill, and matmul #2971

Merged
merged 7 commits into from
Aug 1, 2024

Conversation

jorendumoulin
Copy link
Collaborator

@jorendumoulin jorendumoulin commented Jul 31, 2024

I happened to need the fill operator and decided to do the matmul as well. This continues the work of #2959
I also made sure that the code works on more than just floating points values.
I moved some duplicate code to determine the type arguments of the hidden region into a common method of the NamedOp base class.
I also used the implicit builder to build the regions as I think this is much more readable.

Doing the matmul was a bit more tricky:

  • for some reason this op prints the attributes in front of the ins and outs
  • generic from includes the linalg.memoized_indexing_maps attribute, which are the indexing maps that would be generated when converting to a linalg

@jorendumoulin jorendumoulin self-assigned this Jul 31, 2024
Copy link

codecov bot commented Jul 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.86%. Comparing base (27e0746) to head (32000bc).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2971      +/-   ##
==========================================
+ Coverage   89.84%   89.86%   +0.01%     
==========================================
  Files         408      408              
  Lines       51028    51053      +25     
  Branches     7912     7919       +7     
==========================================
+ Hits        45848    45878      +30     
+ Misses       3929     3925       -4     
+ Partials     1251     1250       -1     

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


result: Sequence[AnyFloat | IntegerType] = []

for op_type in (op.type for op in operands):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this not just

for op in operands:
  op_type = op.type

?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is probably somewhat cleaner

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very minor, but the op_type = op.type line was more to show equivalence rather than meant to be a concrete suggestion. Happy either way though!

xdsl/dialects/linalg.py Outdated Show resolved Hide resolved
xdsl/dialects/linalg.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@n-io n-io left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks good!

@jorendumoulin jorendumoulin merged commit 3543d82 into main Aug 1, 2024
10 checks passed
@jorendumoulin jorendumoulin deleted the joren/generic-printing branch August 1, 2024 11:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants