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: (math) Use SameOperandsAndResultType trait #3761

Open
wants to merge 2 commits into
base: christos/core/traits/same-operands-and-result-types
Choose a base branch
from

Conversation

compor
Copy link
Collaborator

@compor compor commented Jan 17, 2025

This PR:

  • Uses the SameOperandsAndResultType trait in the math dialect
  • Simplifies relevant test cases which were not using the return_type parameter

Stacked on: #3751

@compor compor added the dialects Changes on the dialects label Jan 17, 2025
@compor compor self-assigned this Jan 17, 2025

def __init__(
self, operand: Operation | SSAValue, fastmath: FastMathFlagsAttr | None = None
):
operand = SSAValue.get(operand)
return super().__init__(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Piggybacked this change as pylint was complaining.

@compor compor force-pushed the christos/dialects/math/use-same-operands-and-result-type-trait branch from b84370e to c97e740 Compare January 17, 2025 14:40
Copy link

codecov bot commented Jan 17, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.26%. Comparing base (c181348) to head (c97e740).

Additional details and impacted files
@@                                   Coverage Diff                                   @@
##           christos/core/traits/same-operands-and-result-types    #3761      +/-   ##
=======================================================================================
- Coverage                                                91.26%   91.26%   -0.01%     
=======================================================================================
  Files                                                      461      461              
  Lines                                                    57454    57433      -21     
  Branches                                                  5545     5545              
=======================================================================================
- Hits                                                     52437    52416      -21     
  Misses                                                    3591     3591              
  Partials                                                  1426     1426              

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

@compor
Copy link
Collaborator Author

compor commented Jan 17, 2025

No sure I understand the CI lockfile fail, but I'll have a look later

Copy link
Member

@superlopuh superlopuh left a comment

Choose a reason for hiding this comment

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

Looks great! What's blocking us from moving math out of experimental?

@compor
Copy link
Collaborator Author

compor commented Jan 17, 2025

Looks great! What's blocking us from moving math out of experimental?

I don't think anything, really, that was my goal with this.
I was just thinking a separate NFC PR for the move, but we can avoid the churn and do this here if you want.

from xdsl.utils.exceptions import VerifyException
from xdsl.utils.test_value import TestSSAValue

_BinOpArgT = TypeVar("_BinOpArgT", bound=Attribute)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is unused. Not sure what the initial outlook here was.

@superlopuh
Copy link
Member

Nice, I think separate PR is good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dialects Changes on the dialects
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants