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.

Sign up for GitHub

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

Jump to bottom

transforms: Add convert-varith-to-arith pass #3309

Merged
merged 14 commits into from
Oct 22, 2024
Merged

transforms: Add convert-varith-to-arith pass #3309

merged 14 commits into from
Oct 22, 2024

Conversation

emmau678
Copy link
Contributor

@emmau678 emmau678 commented Oct 16, 2024 •
edited
Loading

Adding a convert-varith-to-arith pass, which reverses the convert-arith-to-varith pass, rewriting varith operations as a sequence of arith operations.

@emmau678 emmau678 added the transformations Changes or adds a transformatio label Oct 16, 2024
@emmau678 emmau678 self-assigned this Oct 16, 2024
@n-io n-io self-requested a review October 16, 2024 15:11
@codecov Codecov
Copy link

codecov bot commented Oct 16, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.03%. Comparing base (a08d1f8) to head (cd8d479).
Report is 1 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #3309      +/-   ##
==========================================
- Coverage   90.03%   90.03%   -0.01%     
==========================================
  Files         448      448              
  Lines       56346    56370      +24     
  Branches     5403     5405       +2     
==========================================
+ Hits        50731    50752      +21     
- Misses       4189     4191       +2     
- Partials     1426     1427       +1

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

n-io
n-io reviewed Oct 16, 2024
View reviewed changes
xdsl/transforms/varith_transformations.py Outdated Show resolved Hide resolved
xdsl/transforms/varith_transformations.py Outdated Show resolved Hide resolved
emmau678 and others added 3 commits October 21, 2024 13:46
transforms: Add convert-varith-to-arith pass
fe95fb6
@emmau678 @n-io
Specify exact type to match on
be9fb4a 
Co-authored-by: Nicolai Stawinoga <36768051+n-io@users.noreply.github.com>
ruff-format changes
135459b
@emmau678 emmau678 force-pushed the convert_varith_to_arith branch from 26397c7 to 135459b Compare October 21, 2024 12:46
n-io
n-io reviewed Oct 21, 2024
View reviewed changes
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.

Looks really good, thanks for that. There's an edge case where the varith op only has one operand, in which case you could just output the operand without generating an arith op, e.g. replace_matched_op([], new_results=[first_arg])

@emmau678 emmau678 requested a review from n-io October 21, 2024 15:24
n-io
n-io reviewed Oct 22, 2024
View reviewed changes
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.

Looks really good! Just minor things and API suggestions left

xdsl/transforms/varith_transformations.py Outdated Show resolved Hide resolved
xdsl/transforms/varith_transformations.py Outdated Show resolved Hide resolved
xdsl/transforms/varith_transformations.py Show resolved Hide resolved
xdsl/transforms/varith_transformations.py Outdated Show resolved Hide resolved
emmau678 and others added 5 commits October 22, 2024 10:14
@emmau678 @n-io
Update xdsl/transforms/varith_transformations.py
fae9142 
Co-authored-by: Nicolai Stawinoga <36768051+n-io@users.noreply.github.com>
@emmau678 @n-io
remove GreedyRewritePatternApplier as it is unnecessary
03c1424 
Co-authored-by: Nicolai Stawinoga <36768051+n-io@users.noreply.github.com>
@emmau678 @n-io
remove unnecessary return statement
6872056 
Co-authored-by: Nicolai Stawinoga <36768051+n-io@users.noreply.github.com>
@emmau678
Merge branch 'main' into convert_varith_to_arith
66d8b1e
fix ruff-format error
cd8d479
n-io
n-io approved these changes Oct 22, 2024
View reviewed changes
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.

LGTM, thanks for this

@n-io n-io requested a review from tobiasgrosser October 22, 2024 10:55
@emmau678 emmau678 marked this pull request as ready for review October 22, 2024 11:01
@emmau678 emmau678 merged commit 6c736f7 into main Oct 22, 2024
14 checks passed
@emmau678 emmau678 deleted the convert_varith_to_arith branch October 22, 2024 11:01
@tobiasgrosser
Copy link
Contributor

Nice. Amazing to see this merged so quickly.

@n-io n-io mentioned this pull request Oct 23, 2024
Open
50 tasks
EdmundGoodman pushed a commit to EdmundGoodman/xdsl that referenced this pull request Dec 6, 2024
@emmau678 @n-io @EdmundGoodman
transforms: Add convert-varith-to-arith pass (xdslproject#3309)
942ce3c 
Adding a convert-varith-to-arith pass, which reverses the
convert-arith-to-varith pass, rewriting varith operations as a sequence
of arith operations.

---

---------

Co-authored-by: emmau678 <eu233@Emma-laptop>
Co-authored-by: Nicolai Stawinoga <36768051+n-io@users.noreply.github.com>
Co-authored-by: n-io <n-io@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@n-io n-io n-io approved these changes

@tobiasgrosser tobiasgrosser Awaiting requested review from tobiasgrosser

Assignees

@emmau678 emmau678

Labels
transformations Changes or adds a transformatio
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@emmau678 @tobiasgrosser @n-io