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: (arith) Add support for bitcast operation #3805

Merged
merged 10 commits into from
Feb 11, 2025

Conversation

jakedves
Copy link
Contributor

Implementation of MLIR's arith.bitcast op. Test cases are passing but type checking on t1 and t2 variables isn't

xdsl/dialects/arith.py Outdated Show resolved Hide resolved
@jakedves jakedves marked this pull request as draft January 29, 2025 17:03
@jakedves jakedves changed the title dialects: (arith) Add support for bitcast operation [WIP] dialects: (arith) Add support for bitcast operation Jan 29, 2025
Copy link

codecov bot commented Jan 29, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.27%. Comparing base (2338bb0) to head (d60ce88).
Report is 58 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3805      +/-   ##
==========================================
+ Coverage   91.23%   91.27%   +0.03%     
==========================================
  Files         461      462       +1     
  Lines       57487    57719     +232     
  Branches     5548     5569      +21     
==========================================
+ Hits        52449    52683     +234     
+ Misses       3615     3614       -1     
+ Partials     1423     1422       -1     

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

xdsl/dialects/arith.py Outdated Show resolved Hide resolved
xdsl/dialects/arith.py Outdated Show resolved Hide resolved
@jakedves jakedves marked this pull request as ready for review January 29, 2025 17:27
@jakedves jakedves changed the title [WIP] dialects: (arith) Add support for bitcast operation dialects: (arith) Add support for bitcast operation Jan 29, 2025
Copy link
Collaborator

@compor compor left a comment

Choose a reason for hiding this comment

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

Very good, thanks for this. I've left a few comments.

xdsl/dialects/arith.py Outdated Show resolved Hide resolved
xdsl/dialects/arith.py Outdated Show resolved Hide resolved
tests/dialects/test_arith.py Outdated Show resolved Hide resolved
xdsl/dialects/arith.py Outdated Show resolved Hide resolved
xdsl/dialects/arith.py Outdated Show resolved Hide resolved
@compor compor requested a review from superlopuh January 30, 2025 07:51
@compor compor added the dialects Changes on the dialects label Jan 30, 2025
tests/dialects/test_arith.py Outdated Show resolved Hide resolved
xdsl/dialects/arith.py Outdated Show resolved Hide resolved
xdsl/dialects/arith.py Outdated Show resolved Hide resolved
xdsl/dialects/arith.py Outdated Show resolved Hide resolved
xdsl/dialects/arith.py Outdated Show resolved Hide resolved
@jakedves
Copy link
Contributor Author

jakedves commented Feb 4, 2025

I've made all the changes suggested besides moving that function to xdsl.utils.type because it handles an edge case (IndexType) that I don't think would be useful/apply to other ops. Renamed it to make it clearer that its not generic if that's alright?

Copy link
Collaborator

@compor compor left a comment

Choose a reason for hiding this comment

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

A few nits!

tests/dialects/test_arith.py Outdated Show resolved Hide resolved
tests/dialects/test_arith.py Outdated Show resolved Hide resolved
xdsl/dialects/arith.py Outdated Show resolved Hide resolved
xdsl/dialects/arith.py Outdated Show resolved Hide resolved
tests/dialects/test_arith.py Outdated Show resolved Hide resolved
tests/dialects/test_arith.py Outdated Show resolved Hide resolved
xdsl/dialects/arith.py Outdated Show resolved Hide resolved
@superlopuh
Copy link
Member

@jakedves, as a convention, we tend to let the original commenter resolve the messages, it makes it easier to see how the change was addressed, and whether the conversation is really resolved.

@superlopuh superlopuh self-requested a review February 5, 2025 09:23
Copy link
Collaborator

@compor compor left a comment

Choose a reason for hiding this comment

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

LGTM modulo a green CI run (the mlir-opt flag has a typo)

@superlopuh
Copy link
Member

Thank you for this!

@compor compor merged commit a297415 into xdslproject:main Feb 11, 2025
16 checks passed
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.

4 participants