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

core: Implement RangeVarConstraint #2704

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open

Conversation

PapyChacal
Copy link
Collaborator

This is similar to VarConstraint, but constraining ranges to match instead.

@PapyChacal PapyChacal added the core xDSL core (ir, textual format, ...) label Jun 10, 2024
@PapyChacal PapyChacal self-assigned this Jun 10, 2024
Copy link

codecov bot commented Jun 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.81%. Comparing base (197ede8) to head (d1d15d7).

Additional details and impacted files
@@                  Coverage Diff                   @@
##           emilien/irdl-split    #2704      +/-   ##
======================================================
+ Coverage               89.79%   89.81%   +0.01%     
======================================================
  Files                     379      379              
  Lines                   48104    48185      +81     
  Branches                 7370     7382      +12     
======================================================
+ Hits                    43197    43278      +81     
- Misses                   3757     3760       +3     
+ Partials                 1150     1147       -3     

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

@PapyChacal PapyChacal marked this pull request as ready for review June 10, 2024 19:31
Copy link
Collaborator

@AntonLydike AntonLydike left a comment

Choose a reason for hiding this comment

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

Some docstrings and comments would be highly welcome! I have no clue what's going on here. I don't even know what a range constraint is.

xdsl/irdl/irdl.py Outdated Show resolved Hide resolved
@PapyChacal
Copy link
Collaborator Author

Some docstrings and comments would be highly welcome! I have no clue what's going on here. I don't even know what a range constraint is.

Added docstrings, and tried to clarify the variable ones! RangeVarConstraint works just like VarConstraint, but on ranges (sequences of attributes, as opposed to single attributes)

Copy link
Collaborator

@math-fehr math-fehr left a comment

Choose a reason for hiding this comment

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

Very nice!

index_operand = TestSSAValue(IndexType())

op = ConstraintRangeVarOp.create(operands=[index_operand], result_types=[i32])
with pytest.raises(DiagnosticException):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you match for part of the exception message?
So we are sure that we are failing because of the range constraint, not something else.

op = ConstraintRangeVarOp.create(
operands=[test_operand], result_types=[TestType("foo")]
)
with pytest.raises(DiagnosticException):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here with the exception message

xdsl/irdl/irdl.py Outdated Show resolved Hide resolved
xdsl/irdl/irdl.py Outdated Show resolved Hide resolved
xdsl/irdl/irdl.py Outdated Show resolved Hide resolved
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@superlopuh superlopuh changed the base branch from main to emilien/irdl-split June 25, 2024 12:03
Base automatically changed from emilien/irdl-split to main June 25, 2024 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core xDSL core (ir, textual format, ...)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants